Chore: Replace entity GRN with infra/grn GRN #74198
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanCech
requested review from
marefr,
oshirohugo,
mildwonkey,
suntala and
yangkb09
and removed request for
a team
DanCech
changed the title
DanCech
added
add to changelog
no-backport
zserge
reviewed
Aug 31, 2023
Comment on lines
+266
to
+268
| bash pkg/plugins/backendplugin/secretsmanagerplugin/generate.sh | ||
| bash pkg/services/store/entity/generate.sh | ||
| bash pkg/infra/grn/generate.sh |
There was a problem hiding this comment.
Shall we maybe have a script that find's all .proto files and applies protoc command to them?
There was a problem hiding this comment.
There is some nuance to the generate.sh files right now, but we could potentially do something like that if we can resolve the differences that we need currently. That sounds like a separate PR though ;)
zserge
reviewed
Aug 31, 2023
Comment on lines
+11
to
+19
| int64 TenantID = 1; | ||
|
|
||
| // The kind of resource being identified, for e.g. "dashboard" or "user". | ||
| // The caller is responsible for validating the value. | ||
| string ResourceKind = 3; | ||
|
|
||
| // ResourceIdentifier is used by the underlying service to identify the | ||
| // resource. | ||
| string ResourceIdentifier = 4; |
There was a problem hiding this comment.
If this is a relatively new and unexposed message - why do we use 1, 3 and 4 and skip the 2 for field IDs?
There was a problem hiding this comment.
I opted to stay consistent with the fields in the entity GRN definition, in theory at least this is wire-compatible even though the field names are different.
zserge
approved these changes
Aug 31, 2023
mildwonkey
approved these changes
Aug 31, 2023
chauchausoup
pushed a commit
to chauchausoup/grafana
that referenced
this pull request
Sep 15, 2023
replace entity GRN with infra/grn GRN
rwwiv
pushed a commit
that referenced
this pull request
Oct 2, 2023
replace entity GRN with infra/grn GRN
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces the entity definition of GRN with the definition from the infra/grn package, and updates the infra/grn package to use a proto definition so that we can import it into the entity proto definition. This allows us to have a single unified understanding of what a GRN is.
To support this I did have to change infra/grn somewhat, the main changes being to use pointers to the GRN structs so that we aren't copying the internal mutexes used by protobuf, and changing the String function to ToGRNString because proto defines String already.