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

feat: use project overarching vars #239

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

Conversation

PierreDelpy
Copy link
Contributor

@PierreDelpy PierreDelpy commented Oct 15, 2024

adapting changes from #221 proposal

@PierreDelpy PierreDelpy requested a review from lablans October 15, 2024 08:29
vars Outdated
Copy link
Member

Choose a reason for hiding this comment

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

BEAM_TAG is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too happy about having different places where the tags are set, but we can tackle that in the future as it touches the Environment topic.

bridgehead Outdated Show resolved Hide resolved
@PierreDelpy PierreDelpy requested a review from TKussel October 15, 2024 09:37
@Threated
Copy link
Member

So I dont know about the future use cases of the top level vars file but I think for now at least a name like versions or something would be better

@PierreDelpy
Copy link
Contributor Author

future use cases of the top level vars file but I think for now at least a name like versions or something would be better

you mean to rename the file or rename the variable names?

bridgehead Outdated
@@ -64,7 +64,7 @@ loadVars() {
fetchVarsFromVaultByFile /etc/bridgehead/$PROJECT.conf || fail_and_report 1 "Unable to fetchVarsFromVaultByFile"
setHostname
optimizeBlazeMemoryUsage
[ -e ./$PROJECT/vars ] && source ./$PROJECT/vars
[ -e ./$PROJECT/vars ] && source ./$PROJECT/vars ./vars
Copy link
Member

@Threated Threated Oct 15, 2024

Choose a reason for hiding this comment

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

I think the source of ./vars should happen before the more specific project vars so that they can override the default and get set regardless of project vars existing or not

@Threated
Copy link
Member

future use cases of the top level vars file but I think for now at least a name like versions or something would be better

you mean to rename the file or rename the variable names?

The file name

Copy link
Member

@Threated Threated left a comment

Choose a reason for hiding this comment

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

Also we should not need the source versions in each vars file as we source it before sourcing $PROJECT/vars

@@ -64,7 +64,7 @@ loadVars() {
fetchVarsFromVaultByFile /etc/bridgehead/$PROJECT.conf || fail_and_report 1 "Unable to fetchVarsFromVaultByFile"
setHostname
optimizeBlazeMemoryUsage
[ -e ./$PROJECT/vars ] && source ./$PROJECT/vars
[ -e ./$PROJECT/vars ] && source ./versions ./$PROJECT/vars
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to bother you again I meant sourcing ./versions regardless of ./$PROJECT/vars existing. I think it always exits so it should not really matter but if we have the check that anyway

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

Successfully merging this pull request may close these issues.

3 participants