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

Qualify all references issue numbers in the materialize repo #29646

Merged
merged 9 commits into from
Sep 20, 2024

Conversation

chaas
Copy link
Contributor

@chaas chaas commented Sep 18, 2024

Do this by adding the materialize# prefix to the issue number.
E.g. Make TODO(#1234) instead TODO(materialize#1234).
This is in preparation for making github issues private. The old links will still work and redirect to the correct issue in the new repo, but the issues will have different ids in the new repo after the migration, so the TODOs should be clear about which repo the number is referring to.

Motivation

This PR refactors existing code. In service of the plans to make Github issues private, and use Github discussions for public tracking instead.

Tips for reviewer

Are there other potential variants of this that I may have missed?

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@chaas chaas marked this pull request as ready for review September 19, 2024 18:58
@chaas chaas requested review from a team as code owners September 19, 2024 18:58
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Thanks!

@def-
Copy link
Contributor

def- commented Sep 19, 2024

Are there other potential variants of this that I may have missed?

We have files ending on .gh<id> to disable them because of an issue. Since they are testdrive files, we can instead use this pattern at the top of the file:

# TODO: Reenable when materialize#<id> is fixed
$ skip-if
SELECT true

Copy link
Contributor

@nrainer-materialize nrainer-materialize left a comment

Choose a reason for hiding this comment

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

There are many occurrences that are not caught. I will try to add some more.

@nrainer-materialize
Copy link
Contributor

nrainer-materialize commented Sep 20, 2024

Rebased and replaced further matches. I only operated on TODOs. There are tons of further issue references in lines without TODOs.

@nrainer-materialize
Copy link
Contributor

nrainer-materialize commented Sep 20, 2024

@def-: I am sure I still haven't caught all of them. Can we emit warnings in the ci-issue-detector when it detects non-qualified issue numbers?

@def-
Copy link
Contributor

def- commented Sep 20, 2024

Sure, will add it. Edit: Done, green run: https://buildkite.com/materialize/nightly/builds/9658

@def- def- force-pushed the qualify-todo-issue-nos branch 2 times, most recently from 2fb21a0 to 4d9a645 Compare September 20, 2024 10:58
And fix found references
@chaas
Copy link
Contributor Author

chaas commented Sep 20, 2024

Thank you so much for the help @def- and @nrainer-materialize .
You're right, searching for the regex (\(| )#[0-9]{4} produces a bunch more results, mostly in test files, and many like # Regression test for #9657.
I can open another PR to change all of them as well, since this one's already quite big.

@chaas chaas merged commit 86e8d85 into MaterializeInc:main Sep 20, 2024
89 checks passed
@chaas chaas deleted the qualify-todo-issue-nos branch September 20, 2024 20:36
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants