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

doc: add the Artifact construction ADR #212

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

Conversation

dirgim
Copy link
Contributor

@dirgim dirgim commented Oct 23, 2024

This ADR aims to cover the proposed functionality for creating additional Artifacts as part of the Konflux build/test/release process.

Signed-off-by: dirgim [email protected]

Signed-off-by: dirgim <[email protected]>

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up to start the discussion (and thanks for writing down some of the thoughts that I brought!). I ran out of time to talk things over more, but I think that it is a good start.


A new API will be introduced which instructs the Integration service to create a Tekton pipeline which will generate the
artifact.
Once built, the artifact reference will be added to the Snapshot's metadata so it can be discovered during testing or release process.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding the artifact to the Snapshot? Is the intention not to replace the Snapshot CR with the produced artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the higher-level purpose of upstreaming and decoupling services, yes, but I think that the Snapshot (or a future replacement API) should still be used by the integration service for the following:

  • As a basis for generating the artifact(s) in the artifact pipelineRuns
  • For running integration testing pipelines

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you are proposing to add it to the metadata, not to the spec (which is where the rest of the references are?). If that is true, I think it makes sense as a link-out to some external storage object. Is another potential option updating the status?

Some further questions assuming the above is true (likely more implementation details than anything):

  • Should we support a list of references to support "rebuilds"?
  • This update should fail if the CR is not present (i.e. if it has been deleted/pruned and stored in some external database). Will this prevent us from being able to rebuild these artifacts?

I agree that some integration-internal resource is likely still needed to be able to construct the artifacts and their pipelineruns. I just expecting that this would effectively require users to migrate their group-like object to no longer be Components. (we can migrate to this flow without requiring migration by auto-configuring a replacement for the "default" snapshot object)

If we are considering to remove the Snapshot as the API to communicate to a release, then we should also consider using the extractor pattern to "pass a snapshot" to integration testing pipelines as well. This will ensure that we have consistency between all tests and release processes. We will, of course, need to plan this migration from submitting a Snapshot for release to submitting some object + extractor to release.

Since the `extractor` resource is a list, it can take multiple extractor entries if the artifact pipeline is expected to provide multiple artifacts.
Each extracted resource will be added to the Snapshot's metadata.

## Consequences
Copy link
Member

Choose a reason for hiding this comment

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

There are many moving parts involved with this change ... and implications about other changes. Is your intention here to isolate into specific incremental proposals? I feel like we need some representation of the end-goal so that we can state how each proposal supports it (and so that we can identify cases where we might be limiting/restricting ourselves).

Some potential relationships:

  • TestSubjectConstructor (related to decoupling integration, implications on build and UI)
  • Remove coupling around an Application and an Application Snapshot (this will have implications for the release service)
  • Potential decrease of a reliance on an Application (this will have implications on build and UI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points, I'll add them to the consequences and think on further implications.


This ADR aims to cover the proposed functionality for creating additional Artifacts as part of the Konflux build/test/release process.
These artifacts would vary in type and would cover use cases where either the single built image or entire sets of images
from the user's application are needed to generate one or more artifacts.
Copy link
Member

Choose a reason for hiding this comment

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

Some limitations with our current approach that can be mitigated with this ADR:

  • The ability to produce multiple artifacts with a single PipelineRun
  • Reduced need for in-PR nudging (one of the common use cases for this is the operator bundle builds as mentioned below)

Comment on lines +41 to +42
Additionally, the integration service will wait for the pipelines to finish before starting the integration testing pipelines
in order to enable users to run tests using the new artifacts.
Copy link
Member

Choose a reason for hiding this comment

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

The pipeline that we are waiting for are the "artifact constructor" ones, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated to clarify.

Comment on lines +71 to +73
extractors:
- ref: 'status.results.filter(result,result.name == "IMAGE_URL")[0].value'
name: ‘metadata.labels["appstudio.openshift.io/component"]’
Copy link
Member

Choose a reason for hiding this comment

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

Is this selector and extractor pattern common with the TestSubject constructor that has been previously proposed? Should it be present separately on multiple CRs or does this indicate a possible need for a common abstraction of the functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I think we are aiming for a similar paradigm to occur in multiple APIs, especially for the selector, but I'm not sure if a common abstraction would be required.

Copy link
Member

Choose a reason for hiding this comment

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

Would these CRDs be managed by the same controller (i.e. integration service) or would there be separate controllers? Would this just be some pattern that could be reused within the same codebase?

Additionally, the integration service will wait for the pipelines to finish before starting the integration testing pipelines
in order to enable users to run tests using the new artifacts.

### The ArtifactConstructor API
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a generic artifact constructor or should it be a more narrowly scoped type of API like a CollectionConstructor?

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'm leaning towards it being a generic artifact constructor to allow for the different types of artifacts to be generated. As long as the pipelines can build something that can be referenced and fetched, I think that we should be able to support it this way.

Copy link
Member

Choose a reason for hiding this comment

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

Are there other use cases that you have in mind?

Comment on lines +69 to +70
- name: pathInRepo
value: pipelines/bundle_generator_pipeline.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This is a pipeline that is specific to generating operator bundles? Is one of the core tenants of the proposal not that we should abstract this further?

Basing this off of the model for building trusted artifacts, would it not be more easily extendible to specify an image which can construct and artifact (collection) based on some set of images passed to it and extract the image(s) into a list? If we construction and extraction logic into tooling, then we can potentially use a common pipeline.

At least one caveat that comes to mind:

  • Build-time tests would be harder to customize on a common pipeline (are there tests that should run in it or should all tests just be run against the produced artifact in an ITS)?

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 may have erred on the side of more generic solution here - I think that we could support both directly building artifacts such as operator bundle images (or any other images), and building OCI artifacts that contain further data/metadata that can be used as described.

kind: ArtifactConstructor
metadata:
name: example--artifact-constructor
spec:
Copy link
Member

Choose a reason for hiding this comment

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

In defining the constructor, we should specify a source/configuration for building the artifact. This would resemble the "normal parameters" which might exist for builds. These parameters will also be needed for the provenance (i.e. so that we can associate the artifacts produced to specific sources).

These artifacts may be desired to be built multi-arch. While a multi-arch pipeline could be selected, the specific platforms to build on would need to be selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would a PARAMS field work here? That way the users could provide their own set of parameters that could also include the source, configuration, etc.

Copy link
Member

@arewm arewm Oct 24, 2024

Choose a reason for hiding this comment

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

Something like that might work. This also relates to the conversation about PAC.


```yaml
apiVersion: appstudio.redhat.com/v2alpha1
kind: ArtifactConstructor
Copy link
Member

Choose a reason for hiding this comment

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

This will result in pipelines run in users' namespaces without the use of PAC. We would lose the ability to interact with PipelineRuns as users might normally be used to (i.e. re-triggering them). How could we ensure that we still provide users with that necessary functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The integration pipelines are also run without the use of PaC, and there is already a re-triggering functionality available for them, would that be enough to cover the use cases?

Copy link
Member

Choose a reason for hiding this comment

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

It might help in some use cases. I don't know how narrowly scoped those are.

Another use case that we need to consider is if the artifact-generating repository reference were updated, we would need to be able to test the new images/build process.

I don't know what the best way to do that is. Would it make sense to still add this Component and then designate some component's repo/configuration as the constructor argument? This feels a little wonky to me.

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