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

Push Snapshots Though Devops Pipelines #2818

Merged
merged 29 commits into from
Sep 27, 2023
Merged

Push Snapshots Though Devops Pipelines #2818

merged 29 commits into from
Sep 27, 2023

Conversation

Pezmc
Copy link
Contributor

@Pezmc Pezmc commented Sep 25, 2023

Description

Marked draft as I'd like some UI feedback from @joepavitt (can be completed in a follow up though!).
Edit: And test failures

Key features:

  • A stage can now be configured on deploy to either create a new snapshot, always copy the existing snapshot, or prompt
  • Deploy snapshot names and descriptions are more intelligent and persist the original details along the pipeline stages
  • User is prompted to select a snapshot on deploy if set as such

Edit Pipeline Stage

Screenshot 2023-09-25 at 16 40 20

Improved Names

Screenshot 2023-09-25 at 16 34 28

Select Snapshot to Deploy

Screenshot 2023-09-25 at 16 33 51

Related Issue(s)

Fixes #2651, #2600

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@joepavitt
Copy link
Contributor

joepavitt commented Sep 26, 2023

I may request an info/help button get added next to the "Select Action" form row label. I'd like to it detail (one/two sentences) each of the options. I've managed to decipher what each of them means, but not sure it would be intuitive to all users.

This is why I should be using the factory here...
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #2818 (85f0f03) into main (b481f64) will decrease coverage by 0.06%.
Report is 129 commits behind head on main.
The diff coverage is 38.88%.

@@            Coverage Diff             @@
##             main    #2818      +/-   ##
==========================================
- Coverage   39.82%   39.76%   -0.06%     
==========================================
  Files         540      556      +16     
  Lines       19215    19837     +622     
  Branches     4551     4705     +154     
==========================================
+ Hits         7653     7889     +236     
- Misses      11562    11948     +386     
Flag Coverage Δ
backend 75.52% <79.71%> (+0.62%) ⬆️
frontend 2.08% <0.69%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
forge/db/models/Project.js 97.97% <100.00%> (+0.04%) ⬆️
forge/ee/db/models/PipelineStage.js 97.36% <100.00%> (+0.14%) ⬆️
forge/ee/db/views/PipelineStage.js 96.55% <ø> (ø)
...end/src/pages/application/PipelineStage/create.vue 0.00% <ø> (ø)
...ntend/src/pages/application/PipelineStage/edit.vue 0.00% <ø> (ø)
frontend/src/pages/instance/Snapshots/index.vue 0.00% <ø> (ø)
forge/services/snapshots.js 96.49% <96.77%> (-0.74%) ⬇️
frontend/src/api/pipeline.js 80.76% <20.00%> (-10.54%) ⬇️
...ations/20230919-01-add-action-to-pipeline-stage.js 0.00% <0.00%> (ø)
...ntend/src/pages/application/PipelineStage/form.vue 0.00% <0.00%> (ø)
... and 3 more

... and 86 files with indirect coverage changes

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Looks good on the backend side. One small update needed around logging.

Also to note we have two active PRs with migrations included - this one and #2738.

As it stands, #2738 must be merged first to ensure the migration ordering is preserved. That is the order I'm expecting these PRs to land in. If, for any reason, 2738 is delayed, we'll need to fixup the migration filenames.

forge/services/snapshots.js Outdated Show resolved Hide resolved
@Pezmc Pezmc marked this pull request as ready for review September 27, 2023 12:34
@knolleary knolleary self-requested a review September 27, 2023 12:44
@knolleary
Copy link
Member

I think it would be helpful to have the configured action displayed on the stage:

image

Not going to block this PR for that... but would be great to get a quick follow-up that adds it.

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Slight rewording for the dialog

frontend/src/pages/application/PipelineStage/form.vue Outdated Show resolved Hide resolved
Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
@Pezmc
Copy link
Contributor Author

Pezmc commented Sep 27, 2023

@knolleary Screenshot 2023-09-27 at 17 08 59

@Pezmc
Copy link
Contributor Author

Pezmc commented Sep 27, 2023

Screenshot 2023-09-27 at 17 13 40
Screenshot 2023-09-27 at 17 13 26
Screenshot 2023-09-27 at 17 14 11

@knolleary
Copy link
Member

@Pezmc looks good to me

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Merge when green

@Pezmc
Copy link
Contributor Author

Pezmc commented Sep 27, 2023

The test failure is not related to this PR and can be seen on main as far back as https://github.com/flowforge/flowforge/pull/2738/files, @hardillb and I are looking into the root cause, but there's no need to block this PR so merging away.

@Pezmc Pezmc merged commit fde74fa into main Sep 27, 2023
3 of 5 checks passed
@Pezmc Pezmc deleted the feat-2651-push-snapshots branch September 27, 2023 16:09
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.

Push Snapshots Through DevOps Pipeline
3 participants