-
Notifications
You must be signed in to change notification settings - Fork 1
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
ci as a state of the art, part 2 #20
Conversation
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Infrastructure as Code | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Vulnerabilities | ![]() ![]() ![]() ![]() |
View in Orca |
goos: 'darwin' | ||
- goarch: 'arm' | ||
goos: 'darwin' | ||
uses: ./.github/workflows/50_go_build-one-arch-one-os-one-path.yml |
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.
I don't see a big benefit to splitting .github/workflows/50_go_build-one-component.yml
and .github/workflows/50_go_build-one-arch-one-os-one-path.yml
. I'd keep them in one workflow.
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.
My current thoughts are:
build-one-arch-one-os-one-path
is already pretty big, and with assumption unit testing and packing into Docker will be added there, will make it even more complicated- so my preference were not overcomplicate it as much as possible
- I also keep an idea in my head, to make each workflow useful by itself.
- and thinking about next scenario: we have workflow for regular PR, when we want by the end to have containers for all the aarchs we use in production
- for the PRs in 'Draft' mode, dev may want faster feedback, in charge of not all the archs build.
- For that
build-one-arch
can be good fit, while keeping50_go_build-one-component
for the main workflow - also I have an idea of adding arch-os presets, so for example user can set: build for linux only. or having workflow for
i386
only.. that will add some complexity into50_go_build-one-component
workflow, and I'd like to have it separated
With the items 1 and 2 as main reasons, 7 as an extra reason, and 3-6 as nice bonus, do you think we could keep it that way for now?
If we found nothing from 1, 2 or 7 useful in practice, it will be easy to combine them in one.
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.
is already pretty big
it's ~10% of build_and_test.yml from the neon repo, I think we can bare a few more lines here.
will make it even more complicated
I don't think it's going to be more complicated, if anything I think it'd be less complicated because you don't have to jump through as many files to understand what's going on
for the PRs in 'Draft' mode, dev may want faster feedback, in charge of not all the archs build
We can do that without splitting it into multiple workflows. Look at https://github.com/neondatabase/neon/blob/c8fbbb9b65587d25b9dbd3c8f21266ce07159d02/.github/workflows/pre-merge-checks.yml#L80-L88 vs https://github.com/neondatabase/neon/blob/main/.github/workflows/build_and_test.yml#L165-L171
If we found nothing from 1, 2 or 7 useful in practice, it will be easy to combine them in one.
I think that's the wrong way around: If we find the less complex and less nested solution not to be sufficient, then we should go for the more complex and more nested approach. But I see that this depends on what people perceive as complex. For me, a more nested workflow adds more complexity than a single workflow encompassing more complexity in itself.
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.
ok, with the moving matrix to one-arch-one-os
I encounter next problem working toward adding tests.
- In current setup, I have workflow "build one component" which makes matrix and call
one-arch-one-os
- in
one-arch-one-os
I had preparation steps to build keys and actual build step, that used these keys for fetching cache and actualgo build
- Adding
go test
as step prevents it to run in parallel, and talking to @matyaskuti in our setup build could be 1-2 minutes, and unit tests up to 5-7 minutes, so we definitely want to run them in parallel
and here comes issue with a number of ways to solve it. I don't know yet which one will be best in our case, we will know that when we apply that workflow to our main repos. From the discussion with @matyaskuti for us: "no one size fit all", so there is a chance we will use all of them: either based on component build and amount of tests it have, or based on either it's regular PR or fix, or could be also different to PR vs Draft PR to achieve the best dev experience.
Here are approaches I considered so far:
- Have
one-component
workflow with matrix callingone-arch-one-os
. Insideone-arch-one-os
I have preparation job, that prepare keys, and bothbuild
andtest
jobs needsprepare
. By doing that I'm sure that keys are consistent for the build and for the test and no mistype here could be introduced in the future. - We could move
matrix
to the file withbuild
andtest
jobs. I was thinking about similar approach: having matrix of prepare jobs and each build and test jobs needprepare
job. Just here we have to copy/paste that matrix statement across all 3 jobs:prepare
,build
andtest
. We need also parametrizeneeds
block, but that shouldnt be a big problem. - I could make separate workflows
build
andtest
, maybe even in separate files. And each of them will haveprepare
as a step for a job, and matrix to do the task for set of arches and oses.
Some of these problem will go away when we start using templates/jsonnet for building workflow, so I also expect that part will be changed completly in next 3-4 months.
And being said: as I see we will need to rewrite that part anyway when doing that workflow for our main repos and trying to fit all their requirements and complexity.
So now I keen more just to merge that "part 2" to go further and get sooner to start adding it to our main repos.
Lets sync in Slack on what approach is better for now and for being more flexible for next steps.
after some more thoughts now I see the next approach on build workflows:
I will go that way now, and will submit changes shortly |
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.
with those two nitpicks out of the way it should be good to merge
ok, that branch and PR were started as "lets have a matrix for os and arch, so we can have workflow to cross-build binaries and do it in parallel".
Eventually it evolved to the bigger one, so now it's time to merge it and continue will smaller changes at a time.
What that PR does:
go-mod-cache
andgo-build-cache
.go.mod
andgo.sum
files, and should be updated when these files changedgo-build
cache it's harder to predict what is the best strategy for the key here, so use the approach when for Pull Requests we take cache from the base branch, as we expect PRs close enough to the base branch.linux / {amd64, arm64}
only. Need an option with kind of "preset" so it could be easily adjusted by the user.