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.

Sign up for GitHub

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

Jump to bottom

Added warning when trying to deploy bundle with --fail-if-running and running resources #1163

Merged
merged 6 commits into from
Feb 7, 2024
Merged

Added warning when trying to deploy bundle with --fail-if-running and running resources #1163

merged 6 commits into from
Feb 7, 2024

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Jan 29, 2024 •
edited
Loading

Changes

Deploying bundle when there are bundle resources running at the same time can be disruptive for jobs and pipelines in progress.

With this change during deployment phase (before uploading any resources) if there is --fail-if-running specified DABs will check if there are any resources running and if so, will fail the deployment

Tests

Manual + add tests

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024 •
edited
Loading

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (2f1b81c) 51.51% compared to head (30c3176) 51.42%.
Report is 6 commits behind head on main.

Files Patch % Lines
bundle/deploy/check_running_resources.go 54.02% 34 Missing and 6 partials ⚠️
cmd/bundle/deploy.go 0.00% 6 Missing ⚠️
cmd/bundle/destroy.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1163      +/-   ##
==========================================
- Coverage   51.51%   51.42%   -0.09%     
==========================================
  Files         292      293       +1     
  Lines       16322    16455     +133     
==========================================
+ Hits         8408     8462      +54     
- Misses       7308     7377      +69     
- Partials      606      616      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pietern
pietern reviewed Jan 29, 2024
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

What should we do for continuous jobs/pipelines?

@andrewnester andrewnester changed the title Added warning when trying to deploy bundle with running resources Added warning when trying to deploy bundle with --fail-if-running and running resources Jan 31, 2024
pietern
pietern reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

If a job/pipeline is deleted from the bundle before a deploy I suspect we'll error out because we cannot pull the state for the job/pipeline. If we get a not found error on these API calls we should return that they're not running.

bundle/config/bundle.go Outdated

// FailIfRunning specifies whether to fail the deployment if there are
// running jobs or pipelines in the workspace. Defaults to false.
FailIfRunning bool `json:"fail_if_running,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The place of this field in the overall config seems odd:

bundle:
  fail_if_running: true

It's not clear what this pertains to. What would be a location to make its use more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pietern we can have a Deployment similar to Lock one we have and put it under there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good -- we can consider moving the lock related fields there as well.

W.r.t. the name itself, it is ambiguous what is running: jobs or the deployment itself.

Alternative: fail_on_active_runs or fail_if_runs_active or something.

bundle/deploy/check_running_resources_test.go Outdated Show resolved Hide resolved
bundle/deploy/check_running_resources_test.go Outdated Show resolved Hide resolved
@andrewnester
Copy link
Contributor Author

If a job/pipeline is deleted from the bundle before a deploy I suspect we'll error out because we cannot pull the state for the job/pipeline. If we get a not found error on these API calls we should return that they're not running.

This is already handled when we call isJobRunning and isPipelineRunning, if they are returning an error we skip it and assume job/pipeline are not running

@andrewnester andrewnester added this pull request to the merge queue Feb 7, 2024
Merged via the queue into main with commit 6edab93 Feb 7, 2024
4 checks passed
@andrewnester andrewnester deleted the warning-on-deploy-running-resources branch February 7, 2024 11:27
andrewnester added a commit that referenced this pull request Feb 7, 2024
@andrewnester
Release v0.212.4
50c8752 
Bundles:
 * Allow specifying executable in artifact section and skip bash from WSL ([#1169](#1169)).
 * Added warning when trying to deploy bundle with `--fail-if-running` and running resources ([#1163](#1163)).
 * Group bundle run flags by job and pipeline types ([#1174](#1174)).
 * Make sure grouped flags are added to the command flag set ([#1180](#1180)).
 * Add short_name helper function to bundle init templates ([#1167](#1167)).

Internal:
 * Fix dynamic representation of zero values in maps and slices ([#1154](#1154)).
 * Refactor library to artifact matching to not use pointers ([#1172](#1172)).
 * Harden `dyn.Value` equality check ([#1173](#1173)).
 * Ensure every variable reference is passed to lookup function ([#1176](#1176)).
 * Empty struct should yield empty map in `convert.FromTyped` ([#1177](#1177)).
 * Zero destination struct in `convert.ToTyped` ([#1178](#1178)).
 * Fix integration test with invalid configuration ([#1182](#1182)).
 * Use `acc.WorkspaceTest` helper from bundle integration tests ([#1181](#1181)).
@andrewnester andrewnester mentioned this pull request Feb 7, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 7, 2024
@andrewnester
Release v0.212.4 (#1183)
f6cdc75 
Bundles:
* Allow specifying executable in artifact section and skip bash from WSL
([#1169](#1169)).
* Added warning when trying to deploy bundle with `--fail-if-running`
and running resources
([#1163](#1163)).
* Group bundle run flags by job and pipeline types
([#1174](#1174)).
* Make sure grouped flags are added to the command flag set
([#1180](#1180)).
* Add short_name helper function to bundle init templates
([#1167](#1167)).

Internal:
* Fix dynamic representation of zero values in maps and slices
([#1154](#1154)).
* Refactor library to artifact matching to not use pointers
([#1172](#1172)).
* Harden `dyn.Value` equality check
([#1173](#1173)).
* Ensure every variable reference is passed to lookup function
([#1176](#1176)).
* Empty struct should yield empty map in `convert.FromTyped`
([#1177](#1177)).
* Zero destination struct in `convert.ToTyped`
([#1178](#1178)).
* Fix integration test with invalid configuration
([#1182](#1182)).
* Use `acc.WorkspaceTest` helper from bundle integration tests
([#1181](#1181)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern approved these changes

@shreyas-goenka shreyas-goenka Awaiting requested review from shreyas-goenka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andrewnester @codecov-commenter @pietern @shreyas-goenka