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

Prepare main to become new default branch #9095

Merged
merged 27 commits into from
Jul 29, 2022
Merged

Prepare main to become new default branch #9095

merged 27 commits into from
Jul 29, 2022

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Jul 26, 2022

This PR aims to sync the entire .github folder in main with that in master, updating the configuration for the new repo strategy.

One minor update to the Makefile was necessary in order to keep the tests split into groups, and some updates to some YAML files were necessary to get the linter to pass.

While I was at it, I've now updated the various issue/PR templates to better reflect our current workflow. Please let me know whether those updates seem reasonable. Some of the main changes there:

  • Deleted .github/auto-comment.yml - doesn't seem like it was being used
  • .github/ISSUE_TEMPLATE/{proposal,feature-request}.md - removed the #### For Admin Use section. Doesn't look like we really pay attention to it?
  • .github/ISSUE_TEMPLATE/config.yml - hopefully I've configured this correctly (as per the GitHub docs).
  • .github/ISSUE_TEMPLATE/*.md - updated the instructions in the comments to redirect users to the discussions section of the repo if they want to ask questions instead of logging issues.

@thanethomson thanethomson mentioned this pull request Jul 26, 2022
12 tasks
.github/workflows/e2e-manual.yml Outdated Show resolved Hide resolved
.github/workflows/fuzz-nightly.yml Outdated Show resolved Hide resolved

- name: Archive suppressions
uses: actions/upload-artifact@v3
with:
name: suppressions
path: test/fuzz/**/suppressions
retention-days: 1
retention-days: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that there's any benefit in retaining these results, unless we have a plan to use them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the reasoning in having this in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to keep the crashers for 3 days -- if we are notified on Friday afternoon, we have an opportunity to review on Monday.

But off the top of my head, I don't know what the suppressions are, so I can't speak to why they should or should not be archived.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. As far as I know the fuzz tests are basically useless, (e.g. they've never caught a bug in the last 18 months, as far as I know, aside from sometimes unexpectedly breaking compile, in silly but legitimate ways.)

.github/workflows/jepsen.yml Outdated Show resolved Hide resolved
Makefile
- name: Run Go Tests
run: |
make test-group-${{ matrix.part }} NUM_SPLIT=6
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make sure that the number of splits here (6) are the right number of splits given the runtime of the tests and overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at the runtime of the tests in the patch, and make sure that they're more or less balanced, I think. It's reasonably impressionistic. The way this all works is that it takes a sorted list of all of the packages that have tests and splits them, but number of tests and test runtimes in CI don't split along the same lines, so you could have one group that takes 10 minutes and 5 that take 2, and in that case, you don't actually get any responsiveness by having more splits, because the long poll is in one test.

We always used to have 4 splits, and we upped it to 6 when the tests were more flaky because it meant when you restarted the tests (to be able to merge something) you'd be able to see the failure more easily and potentially reduce the chance that more tests would flake on the restart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for i in {0..5}; do /usr/bin/time -f "Test 0$i: %E" make test-group-0$i NUM_SPLIT=6; done
Test 00: 0:23.59
Test 01: 0:08.11
Test 02: 0:04.93
Test 03: 2:00.23
Test 04: 0:18.79
Test 05: 0:41.22

I think this seems reasonable?

.github/CODEOWNERS Outdated Show resolved Hide resolved
@@ -537,8 +537,7 @@ paths:
type: array
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is fine, but are these changes actually in the v0.34 line? This didn't seem to be related to the CI parts of the change offhand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fixes are just to get the linter to pass. I also went and checked the response schemas against the structs on the main branch and it seems there were a few minor inconsistencies there, so I fixed the schema where it was incorrect.

key: ${{ runner.os }}-${{ github.sha }}-tm-binary
if: env.GIT_DIFF

test_abci_apps:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really no longer relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems as though these were removed in #6684. I'll revert this for now until we re-apply that patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, these tests were moved to the build workflow.

Copy link
Contributor

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but I'll leave approval to those more familiar with the previous CI implementations. Left a few comments on things that stood out to me.

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/linters/markdownlint.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved

- uses: actions/checkout@v3

- name: Install go-fuzz
working-directory: test/fuzz
run: go get -u github.com/dvyukov/go-fuzz/go-fuzz github.com/dvyukov/go-fuzz/go-fuzz-build
run: go install github.com/dvyukov/go-fuzz/go-fuzz@latest github.com/dvyukov/go-fuzz/go-fuzz-build@latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Fuzzing is built in to go1.18, so we should prefer the tooling built into the toolchain. I think that is fine to wait for a later PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was implemented later on toward v0.35/v0.36. We can look at porting that work at some point.


- name: Archive suppressions
uses: actions/upload-artifact@v3
with:
name: suppressions
path: test/fuzz/**/suppressions
retention-days: 1
retention-days: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to keep the crashers for 3 days -- if we are notified on Friday afternoon, we have an opportunity to review on Monday.

But off the top of my head, I don't know what the suppressions are, so I can't speak to why they should or should not be archived.

name: Golang Linter
# Lint runs golangci-lint over the entire Tendermint repository.
#
# This workflow is run on every pull request and push to main.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to mention how to run this locally. (I think it's just make lint?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's just make lint. I've now added this to the end of the comment.

@thanethomson thanethomson marked this pull request as draft July 28, 2022 19:11
@thanethomson thanethomson marked this pull request as ready for review July 28, 2022 20:48
@thanethomson thanethomson changed the title ci: Update main CI config to resemble master's Prepare main to become new default branch Jul 29, 2022
Copy link
Contributor

@tychoish tychoish left a comment

Choose a reason for hiding this comment

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

I think the deletion of the minnet-kubernetes directory is probably fine, but I'm not sure it's in scope for the rest of this PR


/spec @cason @sergio-mena @jmalicevic @milosevic
# Spec related changes can be approved by the protocol design team
/spec @josef-widder @milosevic @cason @sergio-mena @jmalicevic
Copy link
Contributor

Choose a reason for hiding this comment

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

it's potentially a bit confusing to have duplication between this and the full team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?


- name: Run testnets in group ${{ matrix.group }}
- name: Run ${{ matrix.p2p }} p2p testnets
working-directory: test/e2e
run: ./run-multiple.sh networks/nightly/*-group${{ matrix.group }}-*.toml

e2e-nightly-fail-2:
Copy link
Contributor

Choose a reason for hiding this comment

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

should the -2 here also drop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. This is how it is on master:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it. It seems redundant.

# - maligned
# - misspell
- misspell
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of think that we shouldn't change the linter config on the branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not changing it, I'm just sync'ing with master. See

- misspell

@@ -2742,6 +2740,33 @@ components:
type: string
example: "Dialing seeds in progress. See /net_info for details"

BlockSearchResponse:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should add this without the corresponding implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already on main (i.e. in v0.34):

type ResultBlockSearch struct {

While fixing the linting issues in the OpenAPI config, I discovered that there were a bunch of things that were missing/incorrect. So I've fixed them here to match the code on main.

See

$ref: "#/components/schemas/BlockSearchResponse"
on main for example, where there's no definition of BlockSearchResponse in the OpenAPI config.

@thanethomson
Copy link
Contributor Author

I think the deletion of the minnet-kubernetes directory is probably fine, but I'm not sure it's in scope for the rest of this PR

To get the YAML linting to pass I either had to fix all of the k8s scripts, or just delete them, so I deleted them (since this is what happened to them on master).

.github/ISSUE_TEMPLATE/bug-report.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/codecov.yml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/fuzz-nightly.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.golangci.yml Outdated Show resolved Hide resolved
tac0turtle and others added 22 commits July 29, 2022 15:13
Remove mintnet as discussed on team call.

closes #1941
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson requested review from a team July 29, 2022 19:15
@thanethomson thanethomson merged commit 85636f6 into main Jul 29, 2022
@thanethomson thanethomson deleted the thane/main-ci branch July 29, 2022 19:20
sergio-mena pushed a commit that referenced this pull request Aug 2, 2022
* Update Makefile with changes from #7372

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Sync main GitHub config with master and update

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unnecesary dot folders

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Sync dotfiles

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unused Jepsen tests for now

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* tools: remove k8s (#6625)

Remove mintnet as discussed on team call.

closes #1941

* Restore nightly fuzz testing of P2P addrbook and pex

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fix YAML lints

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fix YAML formatting nits

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* More YAML nits

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* github: fix linter configuration errors and occluded errors (#6400)

* Minor fixes to OpenAPI spec to sync with structs on main

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove .github/auto-comment.yml - does not appear to be used

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add issue config with link to discussions

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Adjust issue/PR templates to suit current process

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unused RC branch config from release workflow

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fix wildcard matching in build jobs config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document markdownlint config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Restore manual E2E test group config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document linter workflow with local execution instructions

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document and fix minor nit in Super-Linter markdownlint config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Update .github/ISSUE_TEMPLATE/bug-report.md

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

* Update pull request template to add language around discussions/issues

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* .golangci.yml: Deleted commented-out lines

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Drop "-2" from e2e-nightly-fail workflow

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Address triviality concern in PR template

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Sam Kleinman <garen@tychoish.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
sergio-mena added a commit that referenced this pull request Aug 3, 2022
…#9157)

* Update CODEOWNERS to use teams (#9129)

* Update CODEOWNERS to use teams

Update the `CODEOWNERS` file to use the
@tendermint/tendermint-engineering and @tendermint/tendermint-research
teams as opposed to adding people one by one. This makes repository
administration somewhat easier to manage, especially when
onboarding/offboarding people.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add Ethan as superuser

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Prepare `main` to become new default branch (#9095)

* Update Makefile with changes from #7372

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Sync main GitHub config with master and update

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unnecesary dot folders

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Sync dotfiles

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unused Jepsen tests for now

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* tools: remove k8s (#6625)

Remove mintnet as discussed on team call.

closes #1941

* Restore nightly fuzz testing of P2P addrbook and pex

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fix YAML lints

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fix YAML formatting nits

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* More YAML nits

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* github: fix linter configuration errors and occluded errors (#6400)

* Minor fixes to OpenAPI spec to sync with structs on main

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove .github/auto-comment.yml - does not appear to be used

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add issue config with link to discussions

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Adjust issue/PR templates to suit current process

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unused RC branch config from release workflow

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fix wildcard matching in build jobs config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document markdownlint config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Restore manual E2E test group config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document linter workflow with local execution instructions

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document and fix minor nit in Super-Linter markdownlint config

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Update .github/ISSUE_TEMPLATE/bug-report.md

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

* Update pull request template to add language around discussions/issues

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* .golangci.yml: Deleted commented-out lines

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Drop "-2" from e2e-nightly-fail workflow

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Address triviality concern in PR template

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Sam Kleinman <garen@tychoish.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Co-authored-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Sam Kleinman <garen@tychoish.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done/Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants