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

ref(worker): move env var processing to the top #358

Merged

Conversation

technosophos
Copy link
Contributor

This is a minor refactor to move an env var reference to the top.

@technosophos technosophos self-assigned this Mar 12, 2018
@technosophos
Copy link
Contributor Author

I'm trying to figure out how to best bubble this env var reference up to the index.js file. This feels pretty hacky, so if you have any clever ideas, I'd be happy to make this better.

/cc @jchanam and @adamreese

@jchanam
Copy link
Contributor

jchanam commented Mar 13, 2018

I think this is a much better approach. The only thing that I don't know is: Why to have the default value defined twice?

This is what I tried to do on my first approach, but couldn't achieve it :(

@mumoshu
Copy link
Contributor

mumoshu commented Mar 13, 2018

@technosophos Just trying to figure out your intension -
maybe you're going to update this line: https://github.com/Azure/brigade/pull/358/files#diff-d54f19b6bbea870b89fa594044af83d9R158

from

this.serviceAccount = job.serviceAccount || process.env.BRIGADE_SERVICE_ACCOUNT || defaultServiceAccount;

to

this.serviceAccount = job.serviceAccount || defaultServiceAccount;

on the other hand of your current change?

@technosophos
Copy link
Contributor Author

Ugh... that was my mistake, @mumoshu. I need to fix that.

@technosophos technosophos force-pushed the ref/brigade_service_account branch 2 times, most recently from 392c7ef to 0cc95f9 Compare March 16, 2018 00:00
@technosophos
Copy link
Contributor Author

Okay, refactored this based on some OTS input from @adamreese . This new version should be a little bit more future-proof, and is based on an idea he found in ts-node.

@technosophos technosophos changed the title WIP ref(worker): move env var processing to the top ref(worker): move env var processing to the top Mar 16, 2018
Copy link
Contributor

@adamreese adamreese left a comment

Choose a reason for hiding this comment

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

Don't forget to run make format

@@ -142,18 +166,18 @@ export class JobRunner implements jobs.JobRunner {
event: BrigadeEvent;
job: jobs.Job;
client: kubernetes.Core_v1Api;
localOptions: KubernetesOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just call this options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

This is a minor refactor to move an env var reference to the top.
@technosophos technosophos force-pushed the ref/brigade_service_account branch from 0cc95f9 to b1bd071 Compare March 16, 2018 22:20
@technosophos technosophos merged commit 7006d90 into brigadecore:master Mar 16, 2018
@technosophos technosophos deleted the ref/brigade_service_account branch March 16, 2018 22:29
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.

4 participants