-
Notifications
You must be signed in to change notification settings - Fork 3
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 SDD for Commodore Compile Pipeline #181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General observations / remarks
- We mention the CI pipeline repo at the beginning, but the rest of the SDD doesn't actually care about that at all, since we just provide a generic "put whatever" field for each tenant repo's CI pipeline configuration.
- Currently it feels like the SDD has two flight-levels: 1) the configuration flight-level which isn't really specific to GitLab (and would most likely work as-is with another Git hosting) and 2) the implementation sketch of how the operator would use the new fields to drive GitLab
Alternative approach for the additional fields in the CRDs
I think that the proposed approach works decently for the narrow scope of the immediate problem, but it seems to me that it tries to unify configs that don't need to be unified while not being very extensible/composable in regards to possible future additions. Therefore, I'd like to propose an alternative approach for configuring the CI pipeline which may be more composable for future extensions at the cost of more upfront work:
Extend GitRepo
with:
spec.accessTokenSecretName
spec.ciVariables
: amap[string]string
Note that extensions to GitRepo
should automatically be reflected in the gitRepoTemplate
fields.
Extend Cluster
and tenant with spec.compilePipeline
Contents of compilePipeline
for Tenant
:
pipelineFiles
: CI configuration to deploy in the tenant repoclusters
: a list of cluster IDs for which the CI pipeline should be configured
Contents of compilePipeline
for Cluster
:
enable
: Boolean which controls whether the CI pipeline should compile this cluster
With this structure:
- The operator generates an access token when it sees
spec.accessTokenSecretName
on a GitRepo and writes the resulting token into a secret with the given name. The operator runs a scheduled job which checks these secrets and refreshes them when they're close to expiring or don't exist anymore in the GitLab API. - The operator writes the contents of each
GitRepo
'sspec.ciVariables
to the GitLab repo as CI/CD variables - The operator updates the tenant's
spec.compilePipeline.clusters
with the cluster's ID when it seescompilePipeline.enable=true
on aCluster
- The operator watches the tenant's
compilePipeline
config and- Generates an entry in the tenant's
gitRepoTemplate.templateFiles
for each entry inpipelineFiles
. Users need to explicitly set the entry inpipelineFiles
to{remove}
to delete the file in the repo. - Writes suitable CI variables into the Tenant's
gitRepoTemplate.ciVariables
to configure the CI pipeline for each cluster incompilePipeline.clusters
- Generates an entry in the tenant's
I see a couple advantages here:
- We get better separation of the configs that only make sense for the Tenant's git repo, the configs that make sense for the
Cluster
and the configs that can be useful for arbitrary Git repos. - We don't tie ourselves to GitLab in the field naming which leaves us in a better position if we want to extend this to non-GitLab Git hosting in the future.
Co-authored-by: Simon Gerber <[email protected]>
Co-authored-by: Simon Gerber <[email protected]>
56dd506
to
fe9f02f
Compare
* `clusters`: List of cluster IDs of clusters for which the compile pipeline should be executed. | ||
This field is managed by the operator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to move this to status.compilePipeline
now that it's intended to be fully operator-managed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're going back to having the pipelineFiles
, it doesn't make sense to fully move this into status anymore. But we might want to split it into configurable vs. operator managed fields and only put the latter in status. I'm not sure what's better practice - keep them together, or have a strict split between managed and user-defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the best practice is here, maybe @bastjan can chime in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer fully operator managed fields in .status
it removes a source of misunderstandings. Not a super firm position since Kubernetes also fully manages a lot of fields in .spec
in the core manifests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split it up now. This prompted me to also add an enabled
field in the spec.
Co-authored-by: Simon Gerber <[email protected]>
fbe2623
to
1a45141
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed and made the title of the SDD more specific for GitLab since there might be a future SDD for GitHub or Forgejo.
* `clusters`: List of cluster IDs of clusters for which the compile pipeline should be executed. | ||
This field is managed by the operator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer fully operator managed fields in .status
it removes a source of misunderstandings. Not a super firm position since Kubernetes also fully manages a lot of fields in .spec
in the core manifests.
Co-authored-by: Sebastian Widmer <[email protected]>
If we actually want to make this SDD title specific to GitLab, we'll also need to rename the source file, otherwise it will look very confusing. Edit: Additionally, I'm not sure this needs to be made specific to GitLab in the title; if there's a future SDD for another Git hosting platform, I'd assume that it would be something along the lines of Edit 2: Overall, my hope is that the structure we come up with here would be applicable for most Git hosting platforms. Afaict we could easily use the exact same structure discussed here to configure GitHub actions, and iirc Gitea/Forgejo actions are modeled after GH actions so that should just work(tm) as well. |
1ccdee8
to
5b4e229
Compare
IMO adding GitLab to the title has the main benefit of making it immediately clear that other git hosts are not yet supported (even though the architecture is generic enough to enable that in the future). |
5b4e229
to
5ed9695
Compare
5ed9695
to
c0b0906
Compare
While that's true, explicitly mentioning GitLab in the title also gives the impression that the contents of this SDD aren't applicable for other Git hosting solutions when they clearly are. |
This reverts commit ac3caef.
@simu you have convinced me. I changed it back, but added a sentence at the start specifying that we're only supporting GitLab for now. |
I agree that it should be generic enough for other Git/CI hosts. I did find the combination of "Commodore Compile Pipeline" and then out-of-scope "any other systems" and the interweaving of generic and GitLab terms a bit confusing. No hard feelings though not making it more specific to GitLab. (The copy-paste error in the title still needs fixing tho 😉) |
Co-authored-by: Simon Gerber <[email protected]>
Checklist