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

Build less frequently #315

Closed
wants to merge 4 commits into from
Closed

Build less frequently #315

wants to merge 4 commits into from

Conversation

colearendt
Copy link
Contributor

@colearendt colearendt commented May 19, 2022

close #255

  • add a more robust computation of directory change
  • add a check for "important_files"
  • ensure that "preview-webhook" is not filtered by diffs (that feels inappropriate)

we ran into a case where a "subdirectory" of a watched directory changing (i.e. my/path, where we watch my/) would not get built. similarly, changing build infrastructure like our "version script" would not trigger new builds either.

A few open questions:

  • should we remove get-diffs.py from the "important_files"
  • should we consider excluding unimportant files rather than including important? i.e. should we err on the side of building too much (if a new file / directory is added) or too little (if a new global / CI affecting file is added and not added to "important_files")
  • is there a better way to think about this problem?
  • should we remove the "build protection" (build pass required to merge) since some builds may not run, now? Maybe a way to have an "outcome" build that succeeds only if all executed builds in the matrix pass, which we can require?
  • should there be a way to ensure that "all builds get run"? Not sure what this would look like... a slash command or something?

TODO: ensure that builds will run on main when they land...

@colearendt colearendt requested a review from atheriel May 19, 2022 09:48
@colearendt colearendt force-pushed the build-less-frequently branch 12 times, most recently from 699ce83 to 3bd808b Compare May 19, 2022 11:29
Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

I don't completely understand why get_diffs.py is doing so many file/directory comparisons. Are we unable to rely on git diff to tell us if a given directory has changed when compared to origin/main?

git diff --quiet origin/main -- connect helper
echo $?

That returns a 0 status when nothing has changed and 1 when there are changes, and is sensitive to changes beneath connect and helper (assuming that's what we need to check).

python-version: '3.9'
architecture: 'x64'
cache: 'pip'
- run: pip install -r requirements.txt
Copy link
Contributor

@bschwedler bschwedler May 19, 2022

Choose a reason for hiding this comment

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

Could we switch over to using urllib instead of requests so we can just use the standard library?

Copy link
Contributor Author

@colearendt colearendt May 19, 2022

Choose a reason for hiding this comment

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

Yes, that's definitely an option! Did I miss something? I thought requests was standard library at one time... but maybe that was old? Or I'm just ignorant and was using a "super standard library"

Copy link
Contributor

@bschwedler bschwedler May 19, 2022

Choose a reason for hiding this comment

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

python2 had urllib, urllib2, and urllib3. requests has an a nicer interface with which to interact, but since we are using it minimally it should be easy to replace with urllib in py3.

@colearendt
Copy link
Contributor Author

colearendt commented May 19, 2022

@aronatkins that's a great call. That is certainly much cleaner 😅 I was mostly cribbing from a pattern that the helm CI tools use, and a bit ignorant of niceties such as the one you mention. A few tricks / challenges that I haven't thought through yet:

  • a way to reference not origin/main, but the previous parent commit (i.e. when running on main). Probably easy enough with a shell command
  • where we do the check. Are we altering the matrix so that fewer jobs actually get launched as we do here, or are we checking at runtime (i.e. launch all the jobs and see whether they should exit immediately or not). I like filtering the matrix, which we could potentially do using this mechanism within the python loop
  • escape hatches for "build everything" (i.e. in the daily cron job for preview, when CI-driving files change like .workflows, etc.)

I do like how elegant that is though 😄 I will revisit and see if I can simplify

add a check for "important_files"

we ran into a case where a "subdirectory" of a watched directory changing (i.e. my/path, where we watch my/) would not get built. similarly, changing build infrastructure like our "version script" would not trigger new builds either.
@colearendt
Copy link
Contributor Author

For the record, since this stagnated. The simple solution was to disable "rebase commits." Squash commits are safe b/c the previous commit was on main, and merge commits are safe b/c the previous commit was on main. Still much work to do teasing through these edge cases 😄

I also am hoping to take a look at the community to see if/how other teams have resolved this issue.

@colearendt colearendt added the cicd Continuous integration/deployment label Aug 15, 2023
@bschwedler
Copy link
Contributor

Closing this out since the PR is based on an older workflow that did not use docker buildx bake.

We still want to revisit the build frequency of these images, which will likely be addressed by the build tooling refactor we are currently working through.

@bschwedler bschwedler closed this Oct 30, 2024
@bschwedler bschwedler deleted the build-less-frequently branch October 30, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cicd Continuous integration/deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build images less, and possibly with additional versioning
3 participants