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

[AM-4684] Add Flink model roles to CLI list/describe commands #2769

Merged
merged 1 commit into from
May 28, 2024

Conversation

lucy-fan
Copy link
Member

What

Adds the Flink model roles to the list and describe roles commands to support the Flink Model EA.

References

Test & Review

@lucy-fan lucy-fan requested a review from a team as a code owner May 23, 2024 22:34
Copy link
Member

@lihaosky lihaosky left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -19,6 +19,7 @@ var (
identityNamespace = optional.NewString("identity")
flinkNamespace = optional.NewString("flink")
workloadNamespace = optional.NewString("workload")
flinkModelNamespace = optional.NewString("flinkmodel")
Copy link
Member

Choose a reason for hiding this comment

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

This is not scaling very well since your team has had to open 9 separate PRs to gradually add all of these namespaces... Has your team considered controlling this list of namespaces in the backend?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We can consider looking into controlling this via the backend but often the CLI and UI have different needs (ie. these roles only need to be available in the CLI for the EA) so doing it via the backend may end up being more work. These roles are scheduled to be in EA this week though so we may not have enough time to figure that out in this change.

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this PR as is, but in the future it would be great to add this to the backend. I assume you could add a switch to distinguish if a request is coming from the UI vs CLI in the backend!

Copy link
Member

Choose a reason for hiding this comment

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

(At the very least, let's create a ticket for this!)

@lucy-fan lucy-fan merged commit c8b7c5c into main May 28, 2024
2 checks passed
@lucy-fan lucy-fan deleted the lfan/flink-model-roles branch May 28, 2024 18:35
@brianstrauch
Copy link
Member

Do you want to add any release notes by the way @lucy-fan? If the changes are early access only, you can prefix the release notes with [Early Access].

@lucy-fan
Copy link
Member Author

@lihaosky Do you want any release notes for the Flink Model role CLI changes?

@lihaosky
Copy link
Member

I guess not since this won't forbid users to create the role binding. Not necessary to advocate it I think.

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.

None yet

3 participants