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

Add a master build docker image for each commit on master branch #5777

Merged
merged 5 commits into from
Jun 14, 2023

Conversation

kamilchodola
Copy link
Contributor

Changes

  • Build docker image with "master" tag on each commit (for tests usage)

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@kamilchodola kamilchodola marked this pull request as ready for review June 5, 2023 13:06
@kamilchodola kamilchodola requested review from a team and rubo as code owners June 5, 2023 13:06
Copy link
Contributor

@FalcoXYZ FalcoXYZ left a comment

Choose a reason for hiding this comment

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

What happens when I push documentation changes to master? Will this trigger a build? If yes, we should reconsider this "build image on master push" trigger.

@kamilchodola
Copy link
Contributor Author

@FalcoXYZ
Docs change is out of repo I think. So it won't trigger it I think.
I think changes which are not touch code are rare enough (maybe we can think of skipping ones which are changing only yamls, not cs files)

@FalcoXYZ
Copy link
Contributor

FalcoXYZ commented Jun 5, 2023

@FalcoXYZ Docs change is out of repo I think. So it won't trigger it I think. I think changes which are not touch code are rare enough (maybe we can think of skipping ones which are changing only yamls, not cs files)

Sorry, I meant anything non-code related. Even if a simple refactor of text (for example) or changing some Github Action.

@kamilchodola
Copy link
Contributor Author

@FalcoXYZ
Yup it will trigger it then - maybe not needed. I can set trigger which will trigger only when cs file is changed liek this:
paths:
- '**.cs'

@kamilchodola
Copy link
Contributor Author

@FalcoXYZ
Added like this:
paths:
- '.cs'
- '
.csproj'
- '.sln'
- '
.config'
- '**.json'

@@ -1,20 +1,35 @@
name: '[BUILD] Docker images and publish'

on:
push:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change to:

push:
  branches: [master]
paths:
  - 'src/Nethermind/**'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to have same think for tests... It triggers every time I commit :/

Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding

- '!src/Nethermind/*.Test'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will limit this one a bit.
but I meant that look below - I'd add some change to yaml and all nethermind unit tests will be triggered - this is worse than those micro optimizations :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you mean you don't wanna trigger Docker build on any test commit. Well, if you mean that each PR commit triggers required tests, then it is what it is. As they are required, they must run anyway. Otherwise, we need to make them not required.

@gat786
Copy link
Member

gat786 commented Jun 6, 2023

If you are building this on every commit on master branch it would make sense to add commit hash to tag of that build so as to quickly be able to view which commit triggered what image when going through a list of images built.

@kamilchodola
Copy link
Contributor Author

@gat786 If I'd add a commit hash to tag, then it would be hard to trigger tests with latest master since I'd not be able to distinguish a name of a docker tag to use in external source.

@gat786
Copy link
Member

gat786 commented Jun 7, 2023

Ok

@kamilchodola kamilchodola merged commit d1f334e into master Jun 14, 2023
@kamilchodola kamilchodola deleted the kch/add_commit_master_docker_build branch June 14, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants