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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

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

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

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.

5 participants