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

Don't build docker containers if we aren't gonna push them #1476

Closed
mstoykov opened this issue May 29, 2020 · 4 comments · Fixed by #1640
Closed

Don't build docker containers if we aren't gonna push them #1476

mstoykov opened this issue May 29, 2020 · 4 comments · Fixed by #1640
Labels
ci evaluation needed proposal needs to be validated or tested before fully implementing it in k6 lower prio

Comments

@mstoykov
Copy link
Contributor

I doubt we will find anything by constantly building them. But we currently do.
This adds another 1m and 30s to each build for no reason IMO.

This can probably wait for us to move to github actions, but maybe it will be fairly easy using the filters ?

@na--
Copy link
Member

na-- commented May 29, 2020

I disagree with the premise of the issue entirely, for historical reasons, github actions or no 😄 I want to always build the docker images, even if we don't push them. This would prevent the situation, which has happened in the past, of git tag-ing a release and then having an error in the Dockerfile, or some stupid Alpine change, or something else, that basically makes it so that you have to make a new release to fix it.

I'd honestly want to go completely in the other direction to what you propose - build all packages all the time, including OS packages and installers, and only push the ones that are for tagged releases. Or maybe not all of the time, but for sure on every master commit... So that actual releases become not very special from a CI perspective.

@na--
Copy link
Member

na-- commented May 29, 2020

Also, the build-docker-images step is not actually required for a PR to be merged, so it's not a blocker for normal everyday work.

@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label May 29, 2020
@mstoykov
Copy link
Contributor Author

Then maybe move it only for it getting build on master?

@na--
Copy link
Member

na-- commented May 29, 2020

Not completely opposed to that, but still seems like a downgrade. For example, you might want to make changes to the Dockerfile or adjacent things (like changing Go versions or messing around with modules, for example)...

Ideally, the docker build step, and other package builds, should happen at the same time as the testing and linting. That way we won't have to wait for them, while still getting the benefit of knowing that packaging isn't broken. And the pushing of the tagged releases should be its own separate step that is triggered only on git tags, and only if all previous steps were successful and error-free. And these push-* steps should reuse whatever binary results the previous build-* steps generated.

If we do this, and I think it can be done with github actions somewhat easily (https://help.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts), then all of the builds can happen for pretty much every commit in every PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci evaluation needed proposal needs to be validated or tested before fully implementing it in k6 lower prio
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants