-
Notifications
You must be signed in to change notification settings - Fork 103
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
Tekton: Migrate from PipelineResources #1218
Conversation
fbb4e31
to
264935e
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.
@Sgitario I tried reviewing the PR but I am missing the context.
It's not clear to me what you are trying to do and why.
Some rough examples:
- Why do we need a clone step ?
- Why in some areas are replacing ouput references for had coded values ?
Note, that I have't touched tekton for nearly two years now, so I am bit rusty.
.withNewMetadata() | ||
.withName(name) | ||
.withAnnotations(this.annotations) | ||
.endMetadata() | ||
.withType(TYPE) | ||
.addToData(DOT_DOCKER_CONFIG_JSON, this.content) | ||
.build(); | ||
.addToData(DOT_DOCKER_CONFIG_JSON, this.content); |
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.
Why don't you just make that key configuralbe? It will save you the doUpate
and extending thing.
Alterntatively, you can just create a decorator that applies the name of they key.
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.
Because we need these two keys. The config.json
is for the step and the .dockerconfigjson
one is the standard for Kubernetes image pull secret.
I wrote a commend in the class that explained why it was required.
* For example, the kaniko image explicitly requires the config.json file. See more in | ||
* https://artifacthub.io/packages/tekton-task/tekton-tasks/kaniko?modal=manifest. | ||
*/ | ||
public class AddDockerConfigJsonSecretDecorator extends io.dekorate.kubernetes.decorator.AddDockerConfigJsonSecretDecorator { |
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.
We shouldn't need to extend this class just to use a different file
name .
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.
Same comment than in #1218 (comment)
We need a git clone step, because the PipelineResources of type git is removed in Tekton 0.47. For the migration, I did what was described in https://tekton.dev/docs/pipelines/resources/ which says to replace the pipelineresources to params and use the
As far as I see, there are no hard-coded values, but default values that users can modify if needed. I don't think this has changed by these changes. But before these properties were provided via resources inputs/outputs, and now via params.
|
Thanks @Sgitario much clearer now. |
No problem! |
annotations/tekton-annotations/src/main/java/io/dekorate/tekton/step/KanikoBuildStep.java
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
@iocanel PR updated with the changes you proposed. |
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
No description provided.