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

Add types & functions to parse env config #76

Merged
merged 5 commits into from
Jan 16, 2025
Merged

Add types & functions to parse env config #76

merged 5 commits into from
Jan 16, 2025

Conversation

zjrgov
Copy link
Contributor

@zjrgov zjrgov commented Jan 15, 2025

🎫 Addresses issue: https://github.com/GSA-TTS/devtools-program/issues/207

Centralized location to manage parsing environment variables for use by different job stages.

🛠 Summary of changes

  • Adds run_env.go & run_env_test.go
  • Struct types to represent relevant portions of VCAP_APPLICATION, JOB_RESPONSE_FILE, and services info.
  • Tests aren't awesome, would be better with a proper table representing a set of different EnvCfg's and various failure states. I'd like to work on them more but unless you, dear reviewer, think it's necessary to do now, I plan to leave it for later so these changes can get in and be built upon by others.

@zjrgov zjrgov requested a review from a team January 15, 2025 17:43
@zjrgov zjrgov self-assigned this Jan 15, 2025
Copy link
Contributor

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

[suggestion (nb)] What would you think about expanding most variable names? JobResponseData instead of JobResData and similar changes. I was having some trouble with remembering what the shortened versions stood for as I read through

@zjrgov
Copy link
Contributor Author

zjrgov commented Jan 16, 2025

I had read Go's conventional style to favor shorter variable names, and so I've been tending to make them shorter than I'm generally accustomed to. In most cases so far I've felt good about it, but I was having the same problem yesterday trying to remember names related to the cloud.gov code and have also been thinking that I have been over-correcting (was planning to update CG stuff to CloudGov).

So I'm a bit on the fence RE "most" names and might prefer to address them more individually, if you don't mind.

In the case of Res, I'm used to working on web servers where req & res are even more common than db for database. Not that I think that's an argument in favor! I don't see it being so common here so I don't have any reason to quibble and am happy to change. But I would generally favor that kind of shortening in wider scopes where something is common and widely understood, like db. So I would default to using what I perceive to be common abbreviations but would change if someone else found it confusing or hard to remember—does that seem like a good balance?

Otherwise I'd adjust based on the usage scope and frequency. So I'd do WorkerDisk -> WorkerDiskSize, WorkerMem -> WorkerMemory, and EnvCfg -> EnvConfig (keeping Env since I'm thinking it's commonly understood) but in the context of a "paragraph" I'd probably still want to opt for cfg (e.g., this test), and short or single letter names for things with tiny scopes and esp. with frequent use (i.e., iterator indices, receiver variables).

@rahearn
Copy link
Contributor

rahearn commented Jan 16, 2025

prefer to address them more individually, if you don't mind.

I don't mind at all.

re: Res in particular, I think being used to res being an http response object was actually part of what was tripping me up about it

@zjrgov
Copy link
Contributor Author

zjrgov commented Jan 16, 2025

re: Res in particular, I think being used to res being an http response object was actually part of what was tripping me up about it

I don't think I understand, I'd have said JobRes is an HTTP response?

@rahearn
Copy link
Contributor

rahearn commented Jan 16, 2025

I don't think I understand, I'd have said JobRes is an HTTP response?

Is it? I was(am) honestly a bit confused about where it came from and how it differs from CUSTOM_CI_

@zjrgov
Copy link
Contributor Author

zjrgov commented Jan 16, 2025

Yeah $JOB_RESPONSE_FILE is a path to a JSON file that contains the body of the response that gitlab-runner gets back from GitLab when requesting a job: https://docs.gitlab.com/runner/executors/custom.html#job-response.

Shall we keep it as JobRes… then?

@zjrgov zjrgov requested a review from rahearn January 16, 2025 16:01
RegPass string `env:"CUSTOM_ENV_CI_REGISTRY_PASSWORD"`
JobImg string `env:"CUSTOM_ENV_CI_JOB_IMAGE"`
JobImg string `env:"CUSTOM_ENV_CI_JOB_IMAGE"`
DockerPass string `env:"CF_DOCKER_PASSWORD"`
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] Do we need this? We set CF_DOCKER_PASSWORD with the value of either DockerHubToken or CIRegistryPass, we don't read it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, nice catch, thanks.

Copy link
Contributor

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

🎉

@zjrgov zjrgov merged commit 124f490 into go/main Jan 16, 2025
1 check passed
@zjrgov zjrgov deleted the go/read-env branch January 16, 2025 20:50
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.

2 participants