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

fix: dockerfile for building proto (#11452) #11703

Merged
merged 4 commits into from
Apr 22, 2022
Merged

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Apr 20, 2022

github.com/regen-network/cosmos-proto/protoc-gen-gocosmos was missing in the Dockerfile

  • fixed the installation with proto cosmos
  • updated dependencies
  • set the go version to 1.17 (latest is already using 1.18)
  • regenerated proto

I know we want to move to buf plugin, but until this is done we have a docker .


All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

github.com/regen-network/cosmos-proto/protoc-gen-gocosmos was missing in the Dockerfile

* fixed the installation with proto cosmos
* updated dependencies
* set the go version to 1.17 (latest is already using 1.18)
* regenerated proto

---

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@robert-zaremba
Copy link
Collaborator Author

merging again the right docker setup again. Tested it locally and works fine.

@marbar3778 reported some past issue, but I was not able to replicate it. would be good if someone else will test is as well.
cc @julienrbrt

@julienrbrt
Copy link
Member

I tested it and it works! However, I wonder why cannot we use directly https://github.com/cosmos/gogoproto like I have tried to do here: #11695

The regen-network repository state that it is deprecated, so why not already move to cosmos one?

(note that for that we need the latest change from master to be tagged: ref cosmos/gogoproto#5 (comment))

@robert-zaremba
Copy link
Collaborator Author

@marbar3778 could you test it? Last time you had some issue.

However, I wonder why cannot we use directly https://github.com/cosmos/gogoproto like I have tried to do here: #11695

Because that causes lots of other changes / incompatibilities, which needs to be handled separately: #11414 .

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

tACK on macos

@alexanderbez
Copy link
Contributor

What's left to get this merged? Or is this merge-able?

@robert-zaremba
Copy link
Collaborator Author

What's left to get this merged? Or is this merge-able?

Marko had some issue with the previous fix... I was not able reproduce it and didn't get more details, so wanted him to to test it.

@robert-zaremba
Copy link
Collaborator Author

@AmauryM test it as well and it worked. So merging.

@robert-zaremba robert-zaremba merged commit bb37e72 into main Apr 22, 2022
@robert-zaremba robert-zaremba deleted the robert/docker-proto branch April 22, 2022 14:11
@robert-zaremba
Copy link
Collaborator Author

now we need to tag 0.7 on the docker hub. @marbar3778 or @alexanderbez can you do that?

@alexanderbez
Copy link
Contributor

now we need to tag 0.7 on the docker hub. @marbar3778 or @alexanderbez can you do that?

Of what image? Also, we should be using ghcr not dockerhub.

If I know the image and how to build it, I can push a tag to ghcr.

@robert-zaremba
Copy link
Collaborator Author

Of what image

tendermintdev/sdk-proto-gen

ghcr not dockerhub.

What's the advantage of ghcr? Maybe let's have a chat on discord?

@faddat
Copy link
Contributor

faddat commented Apr 25, 2022

oh -- ghcr allows it all to be forkable and portable

@alexanderbez
Copy link
Contributor

And it integrates natively into the CI pipeline.

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.

6 participants