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 SDD for Commodore Compile Pipeline #181

Merged
merged 11 commits into from
Jun 21, 2024

Conversation

HappyTetrahedron
Copy link
Contributor

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update the documentation.

@HappyTetrahedron HappyTetrahedron added the documentation Improvements or additions to documentation label Jun 13, 2024
@HappyTetrahedron HappyTetrahedron requested a review from a team as a code owner June 13, 2024 15:13
Copy link
Member

@simu simu left a 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: a map[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 repo
  • clusters: 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.accessTokenSecretNameon 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's spec.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 sees compilePipeline.enable=true on a Cluster
  • The operator watches the tenant's compilePipeline config and
    1. Generates an entry in the tenant's gitRepoTemplate.templateFiles for each entry in pipelineFiles. Users need to explicitly set the entry in pipelineFiles to {remove} to delete the file in the repo.
    2. Writes suitable CI variables into the Tenant's gitRepoTemplate.ciVariables to configure the CI pipeline for each cluster in compilePipeline.clusters

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.

docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
@simu simu requested a review from a team June 17, 2024 08:02
Co-authored-by: Simon Gerber <simon.gerber@vshn.ch>
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
Comment on lines 114 to 115
* `clusters`: List of cluster IDs of clusters for which the compile pipeline should be executed.
This field is managed by the operator.
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

@simu simu Jun 18, 2024

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
Co-authored-by: Simon Gerber <simon.gerber@vshn.ch>
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM overall now.

@simu simu requested a review from bastjan June 19, 2024 07:35
Copy link
Contributor

@bastjan bastjan left a 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.

docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/index.adoc Show resolved Hide resolved
Comment on lines 114 to 115
* `clusters`: List of cluster IDs of clusters for which the compile pipeline should be executed.
This field is managed by the operator.
Copy link
Contributor

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.

docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Widmer <sebastian.widmer@vshn.net>
@simu
Copy link
Member

simu commented Jun 21, 2024

I fixed and made the title of the SDD more specific for GitLab since there might be a future SDD for GitHub or Forgejo.

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 SDD 00XY - Forgejo as GitRepo provider where we can discuss everything that needs to be extended to support this.

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.

@HappyTetrahedron
Copy link
Contributor Author

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).

@simu
Copy link
Member

simu commented Jun 21, 2024

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).

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.

@HappyTetrahedron
Copy link
Contributor Author

HappyTetrahedron commented Jun 21, 2024

@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.

@bastjan
Copy link
Contributor

bastjan commented Jun 21, 2024

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 😉)

docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
docs/modules/SDDs/pages/0032-compile-pipeline.adoc Outdated Show resolved Hide resolved
Co-authored-by: Simon Gerber <simon.gerber@vshn.ch>
@HappyTetrahedron HappyTetrahedron merged commit 973a589 into master Jun 21, 2024
3 checks passed
@HappyTetrahedron HappyTetrahedron deleted the sdd/0032-compile-pipeline branch June 21, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants