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

Added section to Contributing.md detailing our desired pre-release code auditing process #1848

Merged
merged 13 commits into from
Jun 27, 2022

Conversation

xBalbinus
Copy link
Contributor

Closes: #1830

What is the purpose of the change

This PR adds a section to the contributing.md labeled "Major release" detailing our desired pre-release code auditing and actual release workflow processes.

Brief Changelog

Added a section to the contributing.md labeled "Major release" detailing:

The actual release workflow
The pre-release internal auditing process we've discussed as per notes on #1830

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / [Osmosis docs repo](https://github.com/osmosis-labs/docs) / not documented)

This change will be synced up with a future PR that creates an action syncing contributing.md docs on the docs repo with this one.

@xBalbinus xBalbinus requested a review from a team June 21, 2022 18:22
CONTRIBUTING.md Outdated Show resolved Hide resolved
At the moment, we're looking for a tool that lets us statically figure out every message that had something in its code path that changed. Until a tool is found, we must do this manually.

We test in testnet & e2e testnet behaviors about every message that has changed
Copy link
Member

Choose a reason for hiding this comment

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

test in e2e testnet about every message that has changed

I'm all for trying it but are we actually going to test every changed message in e2e before a major release? Is this an achievable commitment in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @ValarDragon for thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I think this requires an extra step of testing but I'm not sure on what your thoughts are on the potential significance of overhead in patches where lots of messages are added (rather, how often these happen). If it's not too significant then it may be do-able.

Copy link
Member

@p0mvn p0mvn Jun 24, 2022

Choose a reason for hiding this comment

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

Please don't feel the need to block the PR on this comment if that's what's happening. I'm just reading between the lines.

I think that the commitment to manually test every changed message on testnet is a must. My concern is only with respect to e2e. Please consider not mentioning it here.

At the moment, we don't have the capacity to test every changed message on the e2e testnet. So the commitment might not be realistic. We should be slowly moving towards that over time so that we can minimize manual testnet testing.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine to list for now - We need to make capacity for testing changed messages on testnet & work towards mitigating # of changes per release / making smaller releases more reasonable imo

CONTRIBUTING.md Outdated
Make sure to also do `make all` to run all the linting tests before you commit and push, as well as `gofmt`-ing the file you've modified or added to make sure everything still abides by the standards.
Copy link
Member

@ValarDragon ValarDragon Jun 22, 2022

Choose a reason for hiding this comment

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

This line should be deleted, as its not part of making proto files. (Also not how I'd suggest getting this integration to folks personally)

At most run make format, but really just set up your IDE to do it for you on-save

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding resources to help people setup their IDE for autosave (and rec to run make format)

@ValarDragon
Copy link
Member

TY for adding this!

@ValarDragon ValarDragon merged commit 6511fcc into main Jun 27, 2022
@ValarDragon ValarDragon deleted the xiangan/contributing branch June 27, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add section to Contributing.md detailing our desired pre-release code auditing process
3 participants