Skip to content

Fix azure protobuf#15864

Merged
GavinFrazar merged 3 commits into
masterfrom
gavinfrazar/fix_azure_proto
Aug 26, 2022
Merged

Fix azure protobuf#15864
GavinFrazar merged 3 commits into
masterfrom
gavinfrazar/fix_azure_proto

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

I made a mistake in #15673 yesterday adding a field to Azure metadata protobuf, giving "ResourceID" the same json key as "Name".

@GavinFrazar GavinFrazar requested review from r0mant and zmb3 August 26, 2022 16:38
@GavinFrazar GavinFrazar marked this pull request as ready for review August 26, 2022 16:38
@GavinFrazar GavinFrazar enabled auto-merge (squash) August 26, 2022 16:43
@github-actions github-actions Bot removed request for nklaassen and zmb3 August 26, 2022 17:01
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Change looks good, but feels like there should be a linter that can catch this and prevent it from happening again. Mind investigating?

@r0mant
Copy link
Copy Markdown
Collaborator

r0mant commented Aug 26, 2022

Wouldn't #15856 catch things like this? cc @codingllama

Although it looks like "legacy" protobufs are ignored there.

@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 26, 2022

I would think it best to have a Go linter that catches this in the generated code, as opposed to trying to teach buf about gogoproto.jsontag.

@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 26, 2022

In fact, looks like go vet catches this:

go vet ./...
# github.com/gravitational/teleport/api/types
types/types.pb.go:1375:2: struct field ResourceID repeats json tag "name" also at types.pb.go:1373

@codingllama
Copy link
Copy Markdown
Contributor

Wouldn't #15856 catch things like this? cc @codingllama

Although it looks like "legacy" protobufs are ignored there.

I don't think linters would go as deep as gogo JSON tags, sadly.

Btw, since we are talking about this, I do have a follow up to enable some linting in legacy protos. Just wait for it. :)

@GavinFrazar GavinFrazar merged commit 7f2c1b1 into master Aug 26, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@GavinFrazar See the table below for backport results.

Branch Result
branch/v10 Failed
branch/v7 Failed
branch/v8 Failed
branch/v9 Failed

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