-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[App] Actually fix launching apps on multiple clusters #15461
Conversation
for more information, see https://pre-commit.ci
I don't think this is what we want. App names must be unique per-project, not per-project + per-cluster @edenafek (I feel strongly about this). If you try to run an app with the same name in a project, targeting a different cluster, the first app needs to be stopped, irrespective of what cluster it is running on. We need to keep the model simple, otherwise the CLI will complicate a ton (now it's not enough to address the app by name, you also need to think whether there's another instance of that app running on another cluster, and instead use the id to address it, e.g. connect, from the CLI). |
👍 I also don't think this is useful for any scenario. Why would you have the same name @edenafek ? If I wanted this as a user I would just create two apps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested change (throughout the system): app names should be unique per project
app_config.cluster_id = most_recent_instance.spec.cluster_id | ||
|
||
# get the existing instance on the cluster, if any | ||
existing_instance = existing_instances_by_cluster.get(app_config.cluster_id, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing_instance = existing_instances_by_cluster.get(app_config.cluster_id, None) | |
existing_instance = existing_instances_by_cluster.get(app_config.cluster_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None
is already the default returning value :)
This was an old PR to prove that we could launch app instances on different clusters. The PR fixing this issue as we discussed is over here: #15484 |
What does this PR do?
Actually fix the cross launching apps issue.
Before
After
Fixes #<issue_number>
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃