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.

Sign up for GitHub

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

Jump to bottom

setup: reuses proto container #9192

Merged
merged 7 commits into from
Apr 26, 2021
Merged

setup: reuses proto container #9192

merged 7 commits into from
Apr 26, 2021

Conversation

robert-zaremba
Copy link
Collaborator

Description

Currently, in makefile we recreate container in every docker call. This is more time an energy (cpu / storage / net) consuming (eg, on every call we need to redownload all package dependencies and setup the environment inside a container). Instead, in this PR I propose to reuse the containers.

closes: #8023

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

robert-zaremba
robert-zaremba commented Apr 23, 2021
View reviewed changes
Makefile Outdated
@@ -369,31 +369,41 @@ devdoc-update:
### Protobuf ###
###############################################################################

containerProtoVer=latest # v0.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

before merging we need to tag a new release for the container, because swagger-gen only works with latest.

Copy link
Member

Choose a reason for hiding this comment

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

you should be able to modify the github action to push once with a tag, then you revert it prior to merging this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that was hack. Can we have a human maintainer for the docker hub?

Copy link
Member

Choose a reason for hiding this comment

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

yea, many people have it. But why not just use the bot? It helps with avoiding the permission hell that arises

@tac0turtle
Copy link
Member

new version https://hub.docker.com/layers/tendermintdev/sdk-proto-gen/v0.2/images/sha256-10f265832540e6ac21a0e053ecfb4cdcd97fd009d00a31060911415bfbe62bd1?context=explore

@tac0turtle
Copy link
Member

@mergify backport

@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2021

Command backport: pending

Waiting for the pull request to get merged

Hey, I reacted but my real name is @Mergifyio

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 26, 2021
@mergify mergify bot merged commit 71b7fb8 into master Apr 26, 2021
@mergify mergify bot deleted the robert/docker branch April 26, 2021 11:15
@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2021

Command backport: failure

No backport have been created
No destination branches found

@amaury1093
Copy link
Contributor

@Mergifyio backport release/v0.42.x

mergify bot pushed a commit that referenced this pull request Apr 26, 2021
@robert-zaremba
setup: reuses proto container (#9192)
e7ac39f 
* setup: reuses proto container

* push docker image

* set image name

* revert and use latest

Co-authored-by: marbar3778 <[email protected]>
Co-authored-by: Amaury <[email protected]>
(cherry picked from commit 71b7fb8)

# Conflicts:
#	Makefile
@mergify mergify bot mentioned this pull request Apr 26, 2021
Merged
@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2021

Command backport release/v0.42.x: success

Backports have been created

alessio pushed a commit that referenced this pull request Apr 28, 2021
@mergify @tac0turtle
@amaury1093 @robert-zaremba
setup: reuses proto container (backport #9192) (#9203)
c6f53ef 
Co-authored-by: marbar3778 <[email protected]>
Co-authored-by: Amaury <[email protected]>
(cherry picked from commit 71b7fb8)
Co-authored-by: Robert Zaremba <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@amaury1093 amaury1093 amaury1093 approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@anilcse anilcse Awaiting requested review from anilcse

@blushi blushi Awaiting requested review from blushi

@fedekunze fedekunze Awaiting requested review from fedekunze

@cyberbono3 cyberbono3 Awaiting requested review from cyberbono3

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reuse containers when using protobuf docker
3 participants
@robert-zaremba @tac0turtle @amaury1093