Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/bash env test #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

msutter
Copy link

@msutter msutter commented Dec 19, 2018

Using the plugin in some docker images produce an 'unbound variable' error by pushing charts to the nexus repository.

You can reproduce this issue by using the 'devth/helm' docker image.

This pull request makes the following changes:

  • Adapt the bash 'if' checks on env vars with the :- notation to set a default value empty string
    (http://wiki.bash-hackers.org/syntax/pe#use_a_default_value)
  • Move the setting of the AUTH env var to include the case were passing user/pw to the push command
  • Add a 1.0.0 version as defined in semantic versioning

@sonatypecla
Copy link

sonatypecla bot commented Dec 19, 2018

Thanks for the contribution! Before we can merge this, we need @msutter to sign the Sonatype Contributor License Agreement.

@DarthHater
Copy link
Member

@dtshepherd wdyt about this one?

@dtshepherd
Copy link
Contributor

Sorry @DarthHater, I've just been absolutely slammed at work lately. This PR looks good and I see an overlapping PR #2. I like this one more because it keeps the set -u, so maybe just add the rename of USERNAME to REPO_USERNAME, so the script doesn't create a conflict with Ubuntu?

@dtshepherd
Copy link
Contributor

dtshepherd commented May 9, 2019

@msutter Did you want to make the change I suggested above so we can get this merged? (sorry for being slow to respond, slammed in my day job...)

@marcoreni
Copy link

@dtshepherd @DarthHater any updates on this? After the changes suggested in #1 (comment) it should be straightforward to merge, right?

@dtshepherd
Copy link
Contributor

Yeah, should be. If I find some spare cycles this week, I can fork this change and apply my suggested fix.

@anton-johansson
Copy link

Any news here? I just upgraded my Nexus to get native Helm chart support, but I got stuck here. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants