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: Add organization environment variables #3856

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rocketeerbkw
Copy link
Member

Draft while coordinating multiple PRs

@shreddedbacon
Copy link
Member

In uselagoon/remote-controller#275 the proposal to add another field to the CRD to support organization variables ties the feature to the release of the remote-controller. I'd prefer to avoid this, and it would be possible to do so.

Since the build-deploy-tool merges everything down into one set of variables that it consumes during the build, it doesn't make a lot of sense to continue doing that and extending CRDs with new fields when we could send the data to the build in a different way. The variable fields in the CRD are just []bytes, which means we're not currently restricted here with what the payload looks like except for what consumes it.

Alternative 1

Do the consolidation on the API side and only send the resulting variables as LAGOON_ENVIRONMENT_VARIABLES (leaving project variables payload empty, essentially deprecating the field, with the side benefit of reducing data sent in the payload in the process). With the possibility of extending the data with a source field that contains where it was sourced from in the event this could be useful in the future (or if there is an existing check in the build-deploy-tool that does care about where it is sourced)

[
	{"name":"ENV_VAR","value":"envvalue","scope": "build","source":"environment"},
	{"name":"PROJ_VAR","value":"projvalue","scope": "build","source":"project"},
	{"name":"ORG_VAR","value":"orgvalue","scope": "build","source":"organization"},
	{"name":"LAGOON_SYSTEM_VAR","value":"sysvalue","scope": "internal_system","source":"internal_system"}
]

This way the merging function in the build-deploy-tool can remain mostly there for backwards compatibility, but would basically be unused.

This would make changes to the build-deploy-tool minimal if it was simply extending variables with the source field. No changes to the remote-controller required (except for maybe a deprecation notice on the project variables field)

Alternative 2

Instead of sending 2 (or 3 in this case) variable payloads for each source type, adjust the way the data is sent to the build from core. Instead of the current []EnvironmentVariable slice for each type in a separate field, use a custom type, or map[string][]EnvironmentVariable. This could make a single payload look something like this:

{
    "environment":[{"name":"ENV_VAR","value":"envvalue","scope": "build"}],
    "project":[{"name":"PROJ_VAR","value":"projvalue","scope": "build"}],
    "organization":[{"name":"ORG_VAR","value":"orgvalue","scope": "build"}],
    "internal_system":[{"name":"LAGOON_SYSTEM_VAR","value":"sysvalue","scope": "internal_system"}]
}

This would then get shipped to the build-deploy-tool as LAGOON_ENVIRONMENT_VARIABLES (again, leaving the project variables empty). All the logic in the build-deploy-tool can still merge variables as required, just the way it would do it would be a bit different with the map. We'd also be able to perform a check in the build to see if the variable payloads received are of the new single payload type, or the older dual payload type. May require a check to allow for backwards compatible processing (ie, if new payload use X merge, if old payloads use Y merge)

This would make changes to the build-deploy-tool still fairly extensive, mostly just modifying what you've already got to instead source from a map. No changes to the remote-controller required either.

My preference would probably be alternative 1, it puts the majority of the changes into core. The variable merging functionality from the build-deploy-tool could even be reproduced in the api-sidecar-handler and benefit from Go tests, and we just call it as required like we do with the ssh key functions (might need some thinking done here, because builds can be triggered by both api and webhooks2tasks. Except webhooks2tasks doesn't have access to this sidecar. The node commons functions would need to know how to communicate with it, meaning the chart would need some adjustments to load the sidecar into the webhooks2tasks service)

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