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

refactor: Cleanup rosetta-ci docker code #10002

Merged
merged 19 commits into from
Sep 13, 2021
Merged

Conversation

fkneeland-figure
Copy link
Contributor

@fkneeland-figure fkneeland-figure commented Aug 24, 2021

Description

Add documentation for rosetta-cli dockerfile and rename folder for the rosetta-ci dockerfile.

Closes: #10001


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-actions github-actions bot added C:Rosetta Issues and PR related to Rosetta Type: Build labels Aug 24, 2021
Copy link
Contributor

@jgimeno jgimeno left a comment

Choose a reason for hiding this comment

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

This is not really true, it is the image used here.

contrib/rosetta/docker-compose.yaml

When we run rosetta tests. If in the future we want to update that image we will need to recreate the dockerfile from scratch.

@fkneeland-figure
Copy link
Contributor Author

@jgimeno I don't think that is correct

If you look at the docker-compose.yaml we are using the official rosetta-cli image for our tests. tendermintdev/rosetta-cli:v0.6.7 Not the dockerfile I deleted. The dockerfile I deleted is not used anywhere, and for this use case I doubt there would be value in having a unique docker image for rosetta-cli that has to be maintained here. If in the future there was a use case which necessitated a custom dockerfile for rosetta-cli then I think it would make more sense to create it then than to keep dead code hanging around that would have to be heavily modified at that point anyways.

The custom dockerfile image that we do use inside of that docker-compose file is rosetta-ci (notice the missing l) and I renamed the folder structure to match that naming convention. When I was working on upgrading my cosmos blockchain to use rosetta I found the naming confusing and I believe that these changes make it a lot easier for a future developer looking at how cosmos uses the rosetta-cli tests to understand what is going on and not get hung up on the unused rosetta-cli/dockerfile or have to look into the makefile to try and figure out how the rosetta-ci image is being created.

Thoughts?

@jgimeno
Copy link
Contributor

jgimeno commented Aug 25, 2021

Hi @fkneeland-figure ! That image tendermintdev/rosetta-cli:v0.6.7 is located in the tendermintdev, is not the official (I think there was not till then). I know because I uploaded it myself.

In theory we should upgrade it everytime there is a new rosetta-cli version.

@fkneeland-figure
Copy link
Contributor Author

@jgimeno Thanks! That makes a lot more sense. I updated the pr, to keep that dockerfile and I added some comments in the rosetta README.md file to explain what it is there for. Should we add a makefile target for updating that docker image on docker hub or do you think that is unnecessary? Thanks for the review!

@fkneeland-figure
Copy link
Contributor Author

@jgimeno you able to look over this one again? Thanks!

Copy link
Contributor

@jgimeno jgimeno left a comment

Choose a reason for hiding this comment

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

Thanks a lot!!!

@fkneeland-figure
Copy link
Contributor Author

@AmauryM @jgimeno do we need anyone else's approval before this can be merged? Thanks!

@jgimeno
Copy link
Contributor

jgimeno commented Sep 3, 2021

@fkneeland-figure
Copy link
Contributor Author

@jgimeno could we get some movement on this? Thanks!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 13, 2021
@mergify mergify bot merged commit 9d58a5a into cosmos:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Rosetta Issues and PR related to Rosetta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup rosetta-ci docker code
3 participants