pipelines-as-code icon indicating copy to clipboard operation
pipelines-as-code copied to clipboard

Bootstrap the repository structure

Open savitaashture opened this issue 1 year ago • 6 comments

Changes

These changes are for onboarding pac using konflux

Signed-off-by: savitaashture [email protected]

Submitter Checklist

  • [ ] 📝 Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).

  • [ ] ♽ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.

  • [ ] ✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).

  • [ ] 📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.

  • [ ] 🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.

  • [ ] 🎁 If feasible, please check if an end-to-end test can be added. See README for more details.

  • [ ] 🔎 If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).

savitaashture avatar Oct 25 '24 07:10 savitaashture

/test linters

savitaashture avatar Oct 25 '24 11:10 savitaashture

do we need a openshift/ directory at the topdir ? can it be in .konflux? I find it weird to have those files in a upstream repository

chmouel avatar Oct 28 '24 08:10 chmouel

do we need a openshift/ directory at the topdir ? can it be in .konflux? I find it weird to have those files in a upstream repository

Today we need it (by convention 😅). We could move them elsewhere (under .konflux for example), we just need to update a bit the tool that generates the payloads…

vdemeester avatar Oct 28 '24 10:10 vdemeester

(by convention 😅). We could move them elsewhere (under .konflux for example), we just need to update a bit the tool that generates the payloads…

i'd rather have the less downstream thing directory and files upstream, i don't think those files are anything useful for the normal folks that don't ship a openshift product

chmouel avatar Oct 28 '24 10:10 chmouel

i'd rather have the less downstream thing directory and files upstream, i don't think those files are anything useful for the normal folks that don't ship a openshift product

Yeah, that make sense 👼🏼

vdemeester avatar Oct 28 '24 11:10 vdemeester

just to clarify i am fine to have a toplevel .konflux with all things downstream in there but not just many directories across the top level...

chmouel avatar Oct 28 '24 11:10 chmouel

@vdemeester as per the discussion moved everything under .konflux PTAL cc @chmouel

savitaashture avatar Nov 04 '24 09:11 savitaashture

some comments are still not addressed:

  • can we have one Dockerfile instead of duplication?
  • can we have it to use the Makefile targets?
  • can we have the top repo Dockerfile that generates our upstream images using those konflux Dockerfile instead or at least have the same kind of compilation/flags etc... if we have this upstream i rather don't have multiple ways in the repo to product images...

chmouel avatar Nov 05 '24 10:11 chmouel

can we have one Dockerfile instead of duplication?

Nope, at least not yet 😓. Those Dockerfile are written once and almost never updated (or automatically), so it's not really an issue.

can we have it to use the Makefile targets?

Those are very very similar to others we have elsewhere and to the one we have downstream.

can we have the top repo Dockerfile that generates our upstream images using those konflux Dockerfile instead or at least have the same kind of compilation/flags etc... if we have this upstream i rather don't have multiple ways in the repo to product images...

Well I was thinking the other way. If we get pac upstream, we just have to remove .konflux (and one .tetkon repository) and we are good to go. The idea being to, at least for now, keep our konflux setup segregated from the "upstream" setup.

vdemeester avatar Nov 05 '24 10:11 vdemeester

can you stash and add a better description of the changes please?

chmouel avatar Nov 19 '24 13:11 chmouel

can you stash and add a better description of the changes please?

@chmouel updated please take a look Thank you

savitaashture avatar Nov 21 '24 13:11 savitaashture

I know it's kind of a nitpick but good software hygiene would be to explain what is konflux in the description of the PR and what it does, just adding "Added necessary changes for onboarding PAC to Konflux." like everyone knows what is Konflux is a bit hasted..

I try to make sure for others when making PR to make a great description of what is intended for someone who is doing reviews or for others that follows the project ie: https://github.com/openshift-pipelines/pipelines-as-code/pull/1826 https://github.com/openshift-pipelines/pipelines-as-code/pull/1825. I would like to make sure we have the same for other PRs (except if it's a one line change trivial fix obv)

chmouel avatar Nov 21 '24 15:11 chmouel

I know it's kind of a nitpick but good software hygiene would be to explain what is konflux in the description of the PR and what it does, just adding "Added necessary changes for onboarding PAC to Konflux." like everyone knows what is Konflux is a bit hasted..

I try to make sure for others when making PR to make a great description of what is intended for someone who is doing reviews or for others that follows the project ie: #1826 #1825. I would like to make sure we have the same for other PRs (except if it's a one line change trivial fix obv)

Agree @chmouel :+1: Updated now i corrected English with chatgpt

savitaashture avatar Nov 22 '24 09:11 savitaashture

sounds good thanks

chmouel avatar Nov 22 '24 09:11 chmouel