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

Snapshots in Pipelines #2399

Merged
merged 38 commits into from Jul 25, 2023
Merged

Snapshots in Pipelines #2399

merged 38 commits into from Jul 25, 2023

Conversation

Pezmc
Copy link
Contributor

@Pezmc Pezmc commented Jul 4, 2023

  • Re-order methods in pipeline file
  • Extract snapshot creation logic into a service
  • Deployment of pipeline stages using snapshots

@hardillb
Copy link
Contributor

hardillb commented Jul 4, 2023

As mentioned when discussing:

  • What happens with credentials as part of the deploy, they should probably be copied over along with the settings and flows. Credential values can be pulled from Env Vars so if done right should "work"

Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Looks like credentials are getting copied if I've read this right

forge/services/snapshots.js Outdated Show resolved Hide resolved
forge/ee/routes/pipeline/index.js Outdated Show resolved Hide resolved
@hardillb
Copy link
Contributor

hardillb commented Jul 5, 2023

Notes from testing

  • - Ticking the "deploy to device" box when creating a step doesn't appear to set the flag in the db, editing the step does set the flag
  • - Not seeing the flow be copied to the second stage
  • - Not seeing the flow being sent to the device (after flag fixed by editing stage)
  • - Last Deployed time appears to never be updated (even with page refresh)

@Pezmc Pezmc force-pushed the feat-2243-pipeline-uses-snapshots branch from e6f6901 to 44407b7 Compare July 5, 2023 12:51
@Pezmc
Copy link
Contributor Author

Pezmc commented Jul 5, 2023

@hardillb / @knolleary Can you start on review of this while I'm finishing the unit test coverage?

@hardillb
Copy link
Contributor

hardillb commented Jul 5, 2023

@Pezmc will do

@knolleary
Copy link
Member

This is all looking good - although we have identified a blocker with the current implementation that will require more time than available to get this in to 1.9 tomorrow.

When promoting between stages, the snapshot is created at the source, copied to target and then applied - all good. However, the snapshot overwrites the target's env var values - where as it should be merging them.

So the step to create the target's copy of the snapshot needs to merge in the target's env vars at that point in time.

A secondary issue I've just spotted, but needs a bit more diagnosis work; I think the credentials object needs to be re-encrypted for the target instance's key, otherwise it cannot decrypt properly.

@Pezmc Pezmc force-pushed the feat-2243-pipeline-uses-snapshots branch from 81bdcf9 to 1f018c6 Compare July 24, 2023 11:01
@Pezmc Pezmc force-pushed the feat-2243-pipeline-uses-snapshots branch from 1f018c6 to b460ba1 Compare July 24, 2023 12:29
@Pezmc Pezmc marked this pull request as ready for review July 24, 2023 14:39
@Pezmc
Copy link
Contributor Author

Pezmc commented Jul 24, 2023

@knolleary @hardillb Theoretically this is ready for review and to be tested locally, but the tests are hanging for a reason I haven't fully explained yet. Please validate it's behaving as you'd expect locally while I get to the bottom of the hanging.

@knolleary
Copy link
Member

Looking good to me locally.

Created a flow using a credential-type env var.

Deployed through the pipeline to another instance, which had access to the env var value.

Confirmed the stored versions (ie encrypted) of the credentials were different - so re-encryption had occurred.

Just want to figure out why the test runs are running away in GH Actions.

@knolleary
Copy link
Member

I've just merged main back to this branch in a hope to get the tests behaving.

@Pezmc
Copy link
Contributor Author

Pezmc commented Jul 24, 2023

@knolleary Still hanging and I really can't explain why... the tests should all have a 5000ms timeout set on them...

@knolleary
Copy link
Member

It feels like the step of the action is just never starting, so not getting as far as have any tests to timeout.

@Pezmc
Copy link
Contributor Author

Pezmc commented Jul 25, 2023

@hardillb @knolleary Tests fixed, PR ready for re-review.

@Pezmc Pezmc requested review from hardillb and knolleary July 25, 2023 08:39
@knolleary knolleary merged commit 65703f1 into main Jul 25, 2023
5 checks passed
@knolleary knolleary deleted the feat-2243-pipeline-uses-snapshots branch July 25, 2023 09:47
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.

None yet

3 participants