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

Set default namespace for velero in env file #22

Merged
merged 3 commits into from
Jan 29, 2021

Conversation

karthikperu7
Copy link
Contributor

No description provided.

env.source.sample Outdated Show resolved Hide resolved
@karthikperu7
Copy link
Contributor Author

Made the default velero ns configurable using env variable in env.sample.source and bashrc_supplement.sh using that env var instead

Copy link
Contributor

@iamkirkbater iamkirkbater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Nice :)

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2021
@karthikperu7 karthikperu7 merged commit 21302f6 into openshift:master Jan 29, 2021
@karthikperu7 karthikperu7 deleted the velero-def branch January 29, 2021 00:35
@drewandersonnz
Copy link
Member

@karthikperu7 should this be set in bashrc/env.source ?? Or should it be in the install-velero.sh script?

As this currently stands, the bashrc is broken for everyone who doesn't realise this change needs to be made to their local env.source. At the very least you would need to default the variable in bashrc or use an if var is set; velero set config.

I'm not sure I'm happy with the location of this code where it is at the moment, can you explain why you chose this location?

@iamkirkbater
Copy link
Contributor

Oh, crap, @drewandersonnz is right. I missed that when I reviewed last night, no more late night code reviews for me.

I wanted to leave this as extensible as possible so that everyone's config can be whatever they want so I asked @karthikperu7 to make the change. While I think it's a good idea to have a default, I missed that the default should have been in the script itself, and not in the env.source.

I'll make another PR to fix that. Good catch, Drew.

@iamkirkbater
Copy link
Contributor

Hmm, although when I stop to think about it, I wonder if it would make sense like you mention, Drew, to put it in install-velero.sh and then have an optional override in the bashrc supplement. So it will already be set in the container (instead of calling it during container init, it's already present) and if you have another need for another default it could be set via env var and set to that upon container init.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants