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

Added Environment variables propagation and mapping feature from ARGOCD_ENV_{VARIABLE} to {VARIABLE} #608

Merged
merged 5 commits into from
Nov 25, 2024

Conversation

kikokikok
Copy link
Contributor

No description provided.

@kikokikok
Copy link
Contributor Author

fixes #607

@kikokikok
Copy link
Contributor Author

First implementation to validate the concept.
Should I gate it with a new LOVELY_ENV_PROPAGATION_ENABLED and LOVELY_ENV_PROPAGATION_FILTER environment variables ?

@Joibel
Copy link
Contributor

Joibel commented Oct 30, 2024

I don't think what you've done here will work as you're expecting.

cmd.Env is not currently set by the existing code, so it would pick up environment variables from the processes environment. Those specified in the application object do NOT get transferred to cmp plugins as is, so the ones you've written in your original issue won't be present - or else they would just work out of the box without your code change. The ones you put in the application get transferred, but they get prefixed with ARGOCD_ENV_ which you're not stripping anywhere.

What I'd have probably implemented is a new parameter (which you weirdly can specify through environment variables) to transfer a bunch of KEY=value environment variables in. I haven't thought this through fully.

@kikokikok
Copy link
Contributor Author

Oh I am not done at all ! I am going through a refactoring to have the Generate method take multiple optional GenerateOption instances, same with the Execute part.
I'm added two Lovely features, one bool to enable env propagation and another with the prefix based filtering of the env variables to propagate. Making it available to all processors and extensible through the GenerateOption and ExecuteOption struct.

@kikokikok
Copy link
Contributor Author

BTW just understood your comment, I finished the refactoring I wanted to introduce so that you guys have an extensibility mechanism for later using GenerateOptions and ExecuteOptions

  • Will now use it to implement the missing piece. Prefix mapper from ARGO_ENV_ to injected ENV if I understood you correctly.

@kikokikok kikokikok force-pushed the feature/env-var-propagation branch from 0f7e78c to a50816f Compare October 31, 2024 16:07
@kikokikok
Copy link
Contributor Author

kikokikok commented Oct 31, 2024

@Joibel I think it should be fine now. I have drastically simplified my approach after getting to know a bit better your code base.
Introduced the Env variables mapping as a feature with LOVELY_ENV_PROPAGATION
And added a test for helmfile to test proper mapping from ARGOCD_ENV_ prefixed env to expected one.
Tested the plugin locally in my argocd setup with my Application, it works as expected

@kikokikok kikokikok changed the title Added Environment variables propagation capability with all/filtered/no propagation. made it backward compatible and opt-in per processor. Added Environment variables propagation and mapping capability from ARGOCD_ENV_{VARIABLE} to {VARIABLE} Oct 31, 2024
@kikokikok kikokikok changed the title Added Environment variables propagation and mapping capability from ARGOCD_ENV_{VARIABLE} to {VARIABLE} Added Environment variables propagation and mapping feature from ARGOCD_ENV_{VARIABLE} to {VARIABLE} Oct 31, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

GoGitOps Review

Grade: A+ (100.0%)

Files: 21

Issues: 0

gofmt: 100%

go_vet: 100%

gocyclo: 100%

golint: 100%

ineffassign: 100%

license: 100%

misspell: 100%

This report was generated using GoGitOps.

@kikokikok kikokikok requested a review from Joibel November 2, 2024 13:16
@kikokikok
Copy link
Contributor Author

Hello @Joibel,

Anything you'd like me to change in the PR, let me know. I tried to minimize the impact as much as possible.
Thanks !

@Joibel
Copy link
Contributor

Joibel commented Nov 5, 2024

Hello @Joibel,

Anything you'd like me to change in the PR, let me know. I tried to minimize the impact as much as possible. Thanks !

Nothing at the moment, I haven't had a chance to look over the PR yet. It's the week before kubecon, which means I'm quite busy, but I will take a look when I get a chance.

Copy link
Contributor

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for doing this

@Joibel Joibel merged commit b768e73 into crumbhole:main Nov 25, 2024
11 of 12 checks passed
@Joibel
Copy link
Contributor

Joibel commented Nov 25, 2024

I am releasing 1.2.0 with this in.

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