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

Implements ResourceWithLabels on AccessListMember #39498

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Mar 18, 2024

No description provided.

@tcsc tcsc requested a review from mdwn March 18, 2024 12:47
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Comment on lines 121 to 122
var _ types.ResourceWithOrigin = (*AccessListMember)(nil)
var _ types.ResourceWithLabels = (*AccessListMember)(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var _ types.ResourceWithOrigin = (*AccessListMember)(nil)
var _ types.ResourceWithLabels = (*AccessListMember)(nil)
var (
_ types.ResourceWithOrigin = (*AccessListMember)(nil)
_ types.ResourceWithLabels = (*AccessListMember)(nil)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

The appearance of the blank identifier in this construct indicates that the declaration exists only for the type checking, not to create a variable. Don't do this for every type that satisfies an interface, though. By convention, such declarations are only used when there are no static conversions already present in the code, which is a rare event.

https://golang.google.cn/doc/effective_go#interfaces_and_types

Do any static conversion already exist? If there aren't and this is to remain, types. ResourceWithOrigin is embedded in types.ResourceWithLabels so I think we can likely reduce this to a single var _ types.ResourceWithLabels = (*AccessListMember)(nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

@espadolini
Copy link
Contributor

@mdwn do access list members have labels, generally speaking?

@mdwn
Copy link
Contributor

mdwn commented Mar 18, 2024

@mdwn do access list members have labels, generally speaking?

They do now that Okta access list sync is a thing.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from capnspacehook March 18, 2024 14:05
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@tcsc tcsc added the no-changelog Indicates that a PR does not require a changelog entry label Mar 18, 2024
@tcsc tcsc enabled auto-merge March 19, 2024 06:17
@tcsc tcsc added this pull request to the merge queue Mar 19, 2024
Merged via the queue into master with commit 81d077e Mar 19, 2024
35 checks passed
@tcsc tcsc deleted the tcsc/acl-member-as-resource-with-labels branch March 19, 2024 06:53
@public-teleport-github-review-bot

@tcsc See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants