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

Accept --location option #105

Merged
merged 15 commits into from
Dec 20, 2023
Merged

Accept --location option #105

merged 15 commits into from
Dec 20, 2023

Conversation

ahangarha
Copy link
Contributor

@ahangarha ahangarha commented Nov 12, 2023

This PR add a -l, --location option to the project, which can be used in the following commands:

  • apply-template
  • ps
  • run
  • run:detached

Question: Should we have CPLN_LOCATION as well? --> Added in ad26c1e
Question: Should we add an option to prevent override by env variable? (like org and app)


Closes #100

Copy link
Collaborator

@rafaelgomesxyz rafaelgomesxyz left a comment

Choose a reason for hiding this comment

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

Just one small thing, but need to change it in multiple places.

lib/command/apply_template.rb Outdated Show resolved Hide resolved
lib/command/ps.rb Outdated Show resolved Hide resolved
lib/command/run.rb Outdated Show resolved Hide resolved
lib/command/run_detached.rb Outdated Show resolved Hide resolved
@ahangarha ahangarha force-pushed the accept-location-option branch 2 times, most recently from 7115de0 to 257b551 Compare November 15, 2023 13:14
@ahangarha
Copy link
Contributor Author

Known issue:

Passing non-exisiting location causes error:

➜  cpl ps --location aws-eu-central-1
rails-7975c5cb68-vftmk
redis-5d9554cd95-czj9b
postgres-0

➜  cpl ps --location aws-us-east-1   
- message: >-
    /org/mostafa-demo/gvc/tutorial-app/workload/rails/deployment/aws-us-east-1
    does not exist
  id: b31c6c3d-3b4c-4277-b73d-1e13cf17817a
  status: 404

@rafaelgomesxyz
Copy link
Collaborator

Let's also add Justin's comment from https://github.com/shakacode/heroku-to-control-plane/pull/92/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R172 here, since this PR is adding the option.

lib/core/config.rb Outdated Show resolved Hide resolved
lib/core/config.rb Outdated Show resolved Hide resolved
@justin808 justin808 marked this pull request as draft November 25, 2023 03:38
@ahangarha
Copy link
Contributor Author

@rafaelgomesxyz Should we check with the ControlPlane actual deployment to ensure the location we use matches the location of the deployed project?

I guess there should be multiple edge cases. We may need to discuss or address those situations in later PRs.

What do you think?


Notice: This PR's implementation may be affected by #110 implementation.

@ahangarha ahangarha marked this pull request as ready for review November 25, 2023 15:49
Copy link
Collaborator

@rafaelgomesxyz rafaelgomesxyz left a comment

Choose a reason for hiding this comment

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

@rafaelgomesxyz Should we check with the ControlPlane actual deployment to ensure the location we use matches the location of the deployed project?

Maybe in another PR, I don't think it fits into the scope of this PR.

@ahangarha ahangarha force-pushed the accept-location-option branch from 51b5e6c to b024d77 Compare December 18, 2023 16:57
Copy link
Collaborator

@rafaelgomesxyz rafaelgomesxyz left a comment

Choose a reason for hiding this comment

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

Some minor changes.

lib/command/base.rb Outdated Show resolved Hide resolved
examples/controlplane.yml Show resolved Hide resolved
lib/core/config.rb Show resolved Hide resolved
@rafaelgomesxyz rafaelgomesxyz merged commit c10625a into main Dec 20, 2023
9 checks passed
@rafaelgomesxyz rafaelgomesxyz deleted the accept-location-option branch December 20, 2023 17:13
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.

Accept location as an option
2 participants