-
Notifications
You must be signed in to change notification settings - Fork 23
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(CLOUDDST-22843): added signtractions python package #156
base: main
Are you sure you want to change the base?
Conversation
@midnightercz please fix the commit/PR title to conform to the gitlint check. Here it should probably be "feat(JIRA-ID): add signtractions python package". Also, can you provide more details? I assume this will be used in a tekton task, so can you provide a link to the related release-service-catalog? It would be nice to have that connection. Also, why is this needed for cosign to be able to sign? Can't cosign sign images on its own? I would assume so. Finally, do I really need to create a gitlab.com account to view the source of that package? If I go to https://gitlab.com/exd-sp-cloud-dist/signtractions it asks me to login. |
@mmalina I'll fix the PR title. I'm about to send PR to release-service-catalog with tekton task and tests. I'll fill the links in description after that. |
4cf3e08
to
0fc8b88
Compare
The commit title needs a lowercase a (added not Added) |
and also preferably "add", not "added" ;)
That's a personal repo. Do you plan on moving this to some Red Hat owned org? |
Right, I'll fix that.
sure, I can move it. Do you have any candidates? There's release-engineering, but I wasn't sure it's the best org for kunflux related stuff |
Signtractions is wrapper for pubtools-sign project which allows signing containers with cosign. This commit is related to the konflux-ci/release-service-catalog#442 which use project signtractions as tekton task for signing containers in release snapshots Signed-off-by: Jindrich Luza <[email protected]>
0fc8b88
to
1c22495
Compare
Hello @midnightercz! Thanks for your contribution. Unfortunately this is not something we want to go forward with. First reason for this and one that we are aiming to weight even more in the future is that this is a personal project. For us, relying in not well-known or Red Hat supported packages/repos/images is a problem as individuals could alter the outcome of any managed Release using those deps. Second reason is the complexity. Although we accept external contributions and we welcome them, at the end of the day we become responsible for maintaining them. In this particular case, the usage of that package as seen in the linked PR requires advance and specific knowledge on the tooling used. This would be a good example. I know this message can be frustrating, specially when you have kindly dedicated your time to contribute to the Release Catalog. You are also an expert in this tool you are proposing and for you this must be an obvious change. Having said that, our ultimately goal is to make the Release process as easy as it can be for the final user and I think this change moves us away from that. |
Hello @davidmogar |
I know you are not an external contributor @midnightercz, but we find ourselves in the same situation: relying on a package from a personal project. This expose security concerns. Regarding the complexity and knowledge required, as you mentioned, we need to know the input to pass to the task. This is complex enough from our point of view, and it would have to be passed in a Release Pipeline that we (Release Team) maintain or, even worse, by users in their ReleasePlanAdmissions. For those reasons, we are rejecting the change as it is. We can schedule a meeting if you want and talk around where to go from here. We are more than happy to support you in writing a change that aligns better with our Pipelines and with the way in which we want users to use them. |
@davidmogar r_signer_wrapper_cosign - will be basically constant, only config file needs to be changed when signing to different envs - which is not your case I assume as you're always signing to quay.io. so to pipeline params only 4 attributes bubble up: i_signing_key, i_snapshot_file, r_signer_wrapper_cosign, r_dst_quay_client. So I would really like to know how this complicates things for end users |
Hello again @midnightercz, I'm going to leave in here some very specific examples of how we do everything in our tasks/pipelines. First of all this is a link to one of our Pipelines. One thing to note in here is that we don't allow arbitrary parameters into our Pipelines. The parameters in there are provided by our operator and will not change. The way in which users provide information is by adding it to the Release, ReleasePlan or ReleasePlanAdmission resources data key. This would be an example:
So yeah, our users are exposed to some complexity but, so far, they are only required to pass short value parameters. Regarding Tasks, this is a good example of a task executing a Python script and consuming the information from the stored resources. At a very least your contribution:
In addition we are not a fan of the way the task is generated or of things like:
You are exposing unnecessary logic. That could be defined in the task like:
Once again, we are more than happy to help you with this. Schedule a meeting with us and we will discuss where to go from here. If not, you have a starting point in this message. |
It would be nice to have this maybe in contributing.md maybe? I have updated konflux-ci/release-service-catalog#442 please check if new style of params is fitting your needs |
@midnightercz This is an old PR with no recent activity. Is it still needed or can it be closed? |
Signtractions is wrapper for pubtools-sign project which allows signing containers with cosign.
This commit is related to the konflux-ci/release-service-catalog#442 which use project signtractions as tekton task for signing containers in release snapshots