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

Add support for dynamic pipeline runs with persistence #12367

Merged
merged 54 commits into from
Apr 9, 2024

Conversation

vyzaldysanchez
Copy link
Contributor

@vyzaldysanchez vyzaldysanchez commented Mar 10, 2024

This PR looks to provide support for dynamic pipeline runs with persistence, by changing the DB structure and adding a new relationship table: job_pipeline_spec.

  • Add new model for job_pipeline_spec.
  • Add corresponding migration.
  • Modify affected code accordingly.
  • Update tests accordingly.

Copy link
Contributor

I see that you haven't updated any README files. Would it make sense to do so?

FROM public.jobs;


ALTER TABLE public.jobs DROP COLUMN pipeline_spec_id; -- Do we use CASCADE here? Does it have any relationship with other tables?
Copy link
Contributor

Choose a reason for hiding this comment

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

I would hold off on dropping this column until a later version -- might be better to deprecate the field first. I might be overly cautious here (I know that we recommend NOPs take a DB backup, but I don't think they all do this reliably.)

@jmank88 Is this a worthwhile precaution to take or not worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bumping @jmank88

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't like the idea of compromising to make up for a lack of backups 🤔
It would be neat if we could do that reliably and generally, but I think there would be many difficulty, messy, and impossible cases. Is this case uniquely important?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's anything uniquely important for this field in specific really.

cc: @jmank88 @cedric-cordenier

@@ -48,7 +49,8 @@ type Runner interface {
// ExecuteAndInsertFinishedRun executes a new run in-memory according to a spec, persists and saves the results.
// It is a combination of ExecuteRun and InsertFinishedRun.
// Note that the spec MUST have a DOT graph for this to work.
ExecuteAndInsertFinishedRun(ctx context.Context, spec Spec, vars Vars, l logger.Logger, saveSuccessfulTaskRuns bool) (runID int64, finalResult FinalResult, err error)
// This will persist the Spec in the DB if it doesn't have an ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@vyzaldysanchez vyzaldysanchez requested review from cedric-cordenier and removed request for a team April 8, 2024 18:31
@vyzaldysanchez vyzaldysanchez added this pull request to the merge queue Apr 9, 2024
Merged via the queue into develop with commit 17d56b8 Apr 9, 2024
108 checks passed
@vyzaldysanchez vyzaldysanchez deleted the task/BCF-2833/dynamic-pipeline-runs branch April 9, 2024 16:01
momentmaker added a commit that referenced this pull request Apr 9, 2024
* develop:
  Update Nix apple sdk usage (#12758)
  [KS-136] Update staging (#12703)
  update changelog path (#12757)
  Add support for dynamic pipeline runs with persistence (#12367)
  Fix nix mac issues building core missing newer apple sdk libs (#12746)
  Fix issues in package.json (#12748)
  [fix] Install jq when building plugins image (#12750)
  Automatic gas in load tests (#12416)
  remove VERSION file to use version from package.json (#12663)
  update package.json version (#12745)
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.

3 participants