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

Cherry pick PR #1498: Add check-xml pre-commit #1502

Closed
wants to merge 2 commits into from

Conversation

cobalt-github-releaser-bot
Copy link
Collaborator

Refer to the original PR: #1498

The driving use case is to validate telemetry xml metadata configuration files, all found under tools/metrics. These are the files used to propagate metadata about the metrics inside Google.

b/296056775

Change-Id: I511a4311b0ce931e7ab3dc945c7b24f3f1a2dc7a

The driving use case is to validate telemetry xml metadata configuration
files, all found under tools/metrics. These are the files used to
propagate metadata about the metrics inside Google.

b/296056775

Change-Id: I511a4311b0ce931e7ab3dc945c7b24f3f1a2dc7a
(cherry picked from commit a7958f3)
@joeltine joeltine marked this pull request as ready for review September 6, 2023 22:29
Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

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

This will fail with conflict markers, we just don't have the lint presubmit workflow in lts branches

The driving use case is to validate telemetry xml metadata configuration
files, all found under tools/metrics. These are the files used to
propagate metadata about the metrics inside Google.

b/296056775

Change-Id: I511a4311b0ce931e7ab3dc945c7b24f3f1a2dc7a
(cherry picked from commit a7958f3)
url
)/
|
components/update_client/((?!cobalt).)*$
|
# Ignore everything under tools/metrics _except_ xml files. We need
# those validated to keep the telemetry/metrics pipeline working.
tools/metrics/(.*\.(?!xml$)[^/]*|[^/.]+)$|
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 this is the only regex you want to add, the rest seem to be unrelated to your original change

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 more than I added originally, but I had to merge conflicts because previous changes to this file were not CP'd into 24.

Copy link
Contributor

Choose a reason for hiding this comment

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

But are the other changes relevant to your change? I would expect this is all you need to add to the regex (though I'd consider changing it directly to the fixed version from #1509). The changes below, for example, only seem to be needed for #1110 which will not be cherry picked to this branch

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Another option is not to CP this at all. Is a presubmit like this necessary in the 24 branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if you don't expect significant changes to xml files on this branch, it makes sense to forgo this change—especially as they'll likely first go through trunk where we have this check in place already

Copy link
Contributor

Choose a reason for hiding this comment

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

K let's skip it.

@joeltine joeltine closed this Sep 20, 2023
@kaidokert kaidokert deleted the 24.lts.1+-1498 branch November 29, 2023 01:35
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.

3 participants