Skip to content

Comments

tooling: Shift project tools to @envoy_repo#25605

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:release-project-fun
Feb 27, 2023
Merged

tooling: Shift project tools to @envoy_repo#25605
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:release-project-fun

Conversation

@phlax
Copy link
Member

@phlax phlax commented Feb 16, 2023

The @envoy_repo repository rule has historically provided a couple of things

  • a PATH to the actual repo for tooling that makes changes to the files
  • version info

recently - added to that is a json "config" that is derived from current project info by the project tooling

this PR groups the "meta" tooling that is used for managing changelogs, syncing, opening/closing branches etc - that is focused on querying or changing the state of the project (as described by the repo)

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 16, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #25605 was opened by phlax.

see: more, trace.

@phlax phlax force-pushed the release-project-fun branch from 86278d4 to 9c11192 Compare February 16, 2023 18:40
@phlax phlax changed the title tooling: Shift project tools to @envoy_repo [WIP] tooling: Shift project tools to @envoy_repo Feb 16, 2023
@phlax phlax marked this pull request as draft February 16, 2023 18:45
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the release-project-fun branch from 9c11192 to 68109fb Compare February 20, 2023 08:16
@phlax phlax changed the title [WIP] tooling: Shift project tools to @envoy_repo tooling: Shift project tools to @envoy_repo Feb 20, 2023
@phlax phlax marked this pull request as ready for review February 20, 2023 08:16
@phlax
Copy link
Member Author

phlax commented Feb 20, 2023

in this PR i have just moved the README, but i intend to follow up and incorporate some of the release info from elsewhere

cc @alyssawilk this makes use of the new proposed maintainer/ folder

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Can you say more about the motivation for moving from "//tools/project" to "@envoy_repo://"? I understand what the former means when I see it ("look for a path named "tool/project" in the root of the Envoy tree) but I'm not as sure what "@envoy_repo" means. But presumably there is a good reason for doing this.

@phlax
Copy link
Member Author

phlax commented Feb 22, 2023

well currently this tooling is a bit obscure - but it is the authoritative way to open/close/etc the repo - so i think it makes more sense to groups these things in the repository rule

this is also a step in trying to unify some of the release documentation with newer workflows etc - and it feels like this would be better categorized as some workflows/methods for maintainers

@RyanTheOptimist
Copy link
Contributor

Hm. Is there a practical difference between "@envoy_repo://foo" and "//tools/project"? Or is the former just "better" in some way? If so, can you can more about why it's better (perhaps in the PR description). You have a lot of context here so I'm certainly inclined to defer to your judgement. At the same time, I've never encountered "@envoy_repo" but have encountered "//tools/" so I'm naturally more inclined to the latter, especially as it appears that switching to "@envoy_repo" requires additional entries in repositories.bzl that we don't need when using "//tools/"

@phlax
Copy link
Member Author

phlax commented Feb 22, 2023

So for context @envoy_repo has historically provided a couple of things

  • a PATH to the actual repo for tooling that makes changes to the files
  • version info

recently - added to that is a json "config" that is derived from current project info by the project tooling (the same tooling that is used for managing changelogs, syncing, opening/closing branches etc)

so basically im trying to group that same tooling as a set of data and tools that is specifically focused on the project (as described by the repo)

technically there are some differences using a repository_rule as is done here but i think none that would preclude doing either way

semantically - separating this tooling here is saying that this is a set a meta targets outside the normal compile targets (and that change/query the state of the project)

i think using the (existing) repo_rule is probs the less controversial part of this PR than my desire to start grouping maintainer docs where i am - altho tbh my intention is mostly as here, to bring together existing docs from other parts of the repo to make them more coherent together, so im not sure thats so controversial either

@RyanTheOptimist
Copy link
Contributor

Ah, thanks! That makes sense. Could you include some of that context in the PR description, for posterity? This seems like a reasonable approach to me. I suspect we should probably get a review from someone more familiar with using these tools, but this approach sounds good to me.

@phlax
Copy link
Member Author

phlax commented Feb 22, 2023

would be good to get a stamp from @alyssawilk re maintainer docs strategy

also worth mentioning is why i added the config object - its so we can generate dockerhub/github etc metatdata from templates and vars

@phlax
Copy link
Member Author

phlax commented Feb 23, 2023

also re using a repository_rule - the reason why this was done in the case of the version info originally is that ordinarily you cannot use information inside repo files (other than BUILD/bzl files) in the bazel rules - this allows us to do that - eg to create versioned packaging rules

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Honestly I have a slightly easier time when the tools live near the code so if something goes sideways I can fix inline. I don't grok to the move to a different repo (especially now we can avoid Envoy CI overhead for tools) but given you do 99% of our tooling I'm happy to sign off on what works for you :-)

@phlax phlax merged commit 328fda7 into envoyproxy:main Feb 27, 2023
@phlax
Copy link
Member Author

phlax commented Mar 1, 2023

/backport 1.23+

backporting this will make the maintainer docs consistent across release branches

@repokitteh-read-only repokitteh-read-only bot added the backport/review Request to backport to stable releases label Mar 1, 2023
phlax added a commit to phlax/envoy that referenced this pull request Mar 1, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
phlax added a commit to phlax/envoy that referenced this pull request Mar 1, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
phlax added a commit that referenced this pull request Mar 1, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
phlax added a commit that referenced this pull request Mar 1, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
phlax added a commit to phlax/envoy that referenced this pull request Mar 1, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
phlax added a commit that referenced this pull request Mar 2, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
@phlax phlax removed the backport/review Request to backport to stable releases label Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants