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

update deployment from file blocks changing cluster #1013

Merged
merged 2 commits into from
Jan 12, 2023

Conversation

jemishp
Copy link
Contributor

@jemishp jemishp commented Jan 12, 2023

Description

This PR detects when an update request using --deployment-file would change the existing deployment's cluster and does not allow that update. Instead it returns an errNotPermitted.

🎟 Issue(s)

Related #1012

🧪 Functional Testing

inspect an existing deployment's cluster

$ astro deployment inspect -n jp-test -k metadata.cluster_id
clbzeum5y000v0tyn8hbv5vsa
$ astro deployment inspect -n jp-test -k configuration.cluster_name
gtadi

request an update to change the cluster fails with an error

jpatel@jemishs-mbp astro-cli % cat deployment.yaml
deployment:
    environment_variables:
        - key: foo
          value: bar
        - key: bar
          value: awesome
          is_secret: true
    configuration:
        name: jp-test
        description: ""
        runtime_version: 7.1.0
        dag_deploy_enabled: false
        executor: KubernetesExecutor
        scheduler_au: 5
        scheduler_count: 1
        cluster_name: team-airflow-operator 
        workspace_name: CLI Test Workspace
    alert_emails:
        - test1@tester.com
        - test2@tester.com
$ astro deployment update --deployment-file deployment.yaml
Error: changing an existing deployment's cluster is not permitted

📸 Screenshots

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Base: 87.72% // Head: 87.74% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (33de9fd) compared to base (8f54066).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
+ Coverage   87.72%   87.74%   +0.02%     
==========================================
  Files         112      112              
  Lines        9923     9926       +3     
==========================================
+ Hits         8705     8710       +5     
+ Misses        711      710       -1     
+ Partials      507      506       -1     
Impacted Files Coverage Δ
cloud/deployment/fromfile/fromfile.go 100.00% <100.00%> (ø)
cmd/cloud/deployment.go 93.30% <100.00%> (+0.83%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jemishp jemishp linked an issue Jan 12, 2023 that may be closed by this pull request
4 tasks
@jemishp jemishp force-pushed the deplyment-file-cluster-change branch 2 times, most recently from e9921ba to 8bc774b Compare January 12, 2023 02:11
- returns `errNotPermitted` if request via file is to modify an existing deployment's cluster
- silencing the usage print from the command sooner so it does not print with the error
@jemishp jemishp force-pushed the deplyment-file-cluster-change branch from 8bc774b to b23e8a7 Compare January 12, 2023 02:44
Copy link
Contributor

@sunkickr sunkickr left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@jemishp jemishp merged commit 286c18b into main Jan 12, 2023
@jemishp jemishp deleted the deplyment-file-cluster-change branch January 12, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return an error when changing an existing deployment's cluster
3 participants