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

[RFC] Stop building Docker images in Pull Requests #1389

Merged
merged 3 commits into from Jun 25, 2020

Conversation

crcrpar
Copy link
Contributor

@crcrpar crcrpar commented Jun 19, 2020

Motivation

Docker build jobs on Pull Request pushes sometimes push down the CI job results which are more critical and required such as tests-python37.
Also, I think we can reckon that Docker build works if CI for Pull Requests pass as these two do almost exactly the same in the installation phase.

Description of the changes

  • Change the trigger of this job
  • Remove unused environment variable
  • Remove if statement

Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I agree with you. Certainly, there is no need to build many docker images every time per push to a PR. However, there may be some developers using the generated images, so I'm concerned that this might be an unfriendly change. Don't we need to investigate how the generated images are being used by developers?

@crcrpar
Copy link
Contributor Author

crcrpar commented Jun 22, 2020

First of all, much appreciate your comment.

However, there may be some developers using the generated images, so I'm concerned that this might be an unfriendly change. Don't we need to investigate how the generated images are being used by developers?

As the workflow shows, currently, there is almost no way to pull docker images built for PRs' commits since we only push the docker images on push to master. So, I don't think we have to do it.

Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the confirmation! If we don't use the created docker images at all, we don't have to create many images in PRs. LGTM for these changes.

@toshihikoyanase toshihikoyanase added the CI Continuous integration. label Jun 23, 2020
@toshihikoyanase toshihikoyanase self-assigned this Jun 23, 2020
@toshihikoyanase
Copy link
Member

Thank you for your PR!
I'm a bit concerned about the review of Dockerfile and setup.py. Is it possible to run the docker build workflow if the PR contains the changes regarding Dockerfile and setup.py?

Trigger Docker build if a PR edits `Dockerfile` or `setup.py` as they have some effects.
@crcrpar
Copy link
Contributor Author

crcrpar commented Jun 24, 2020

I'm a bit concerned about the review of Dockerfile and setup.py. Is it possible to run the docker build workflow if the PR contains the changes regarding Dockerfile and setup.py?

It should be possible per https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-including-paths and gave it a try in 3ce0409.

Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your update!

- master
paths:
- Dockerfile
- setup.py
Copy link
Member

Choose a reason for hiding this comment

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

[Note] The trigger is not limited to the master branch, but I think it is acceptable because the built images are not uploaded to the DockerHub.

@toshihikoyanase toshihikoyanase added this to the v2.0.0 milestone Jun 25, 2020
@toshihikoyanase toshihikoyanase merged commit f527a32 into optuna:master Jun 25, 2020
@crcrpar crcrpar deleted the less-dockerbuild branch June 25, 2020 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants