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

Gracefully handle old pipelines where output commit is ALIAS and meta commit is AUTO #8485

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

acohen4
Copy link
Contributor

@acohen4 acohen4 commented Jan 4, 2023

In #8116 we fixed an issue where ALIAS output commits would get created with corresponding AUTO meta commits. This didn't handle previously faulty DAGs. In this PR, we gracefully handle DAGs already containing the faulty structure.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #8485 (61ca603) into master (aa70e0c) will decrease coverage by 37.42%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #8485       +/-   ##
===========================================
- Coverage   39.78%    2.36%   -37.43%     
===========================================
  Files         455      365       -90     
  Lines      123023   107328    -15695     
===========================================
- Hits        48950     2537    -46413     
- Misses      64420   104446    +40026     
+ Partials     9653      345     -9308     
Impacted Files Coverage Δ
src/server/auth/db.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/log/http.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/log/step.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/watch/op.go 0.00% <0.00%> (-100.00%) ⬇️
src/server/license/db.go 0.00% <0.00%> (-100.00%) ⬇️
src/server/identity/db.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/log/amazon.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/log/stdlog.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/testutil/db.go 0.00% <0.00%> (-100.00%) ⬇️
src/internal/dbutil/option.go 0.00% <0.00%> (-100.00%) ⬇️
... and 403 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -107,7 +107,8 @@ func (pj *pendingJob) load() error {
return errors.EnsureStack(err)
}
// both commits must have succeeded - a validation error will only show up in the output
if metaCI.Error == "" && outputCI.Error == "" {
// the commit must also not be of type ALIAS, to ensure that we have a corresponding job
if metaCI.Error == "" && outputCI.Error == "" && outputCI.Origin.Kind != pfs.OriginKind_ALIAS {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add a log message around line 117 so we know which commit we picked as the base commit? Maybe it gets recorded elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea!

@acohen4 acohen4 force-pushed the acohen4/handle-alias-output-commit branch from 6f2e606 to 4f6f7e1 Compare January 4, 2023 22:56
@acohen4 acohen4 merged commit 0606417 into master Jan 5, 2023
@acohen4 acohen4 deleted the acohen4/handle-alias-output-commit branch January 5, 2023 11:20
acohen4 added a commit that referenced this pull request Jan 5, 2023
… commit is AUTO (#8485)

Gracefully handle old pipelines where output commit is ALIAS and meta commit is AUTO
acohen4 added a commit that referenced this pull request Jan 5, 2023
… commit is AUTO (#8485) (#8493)

Gracefully handle old pipelines where output commit is ALIAS and meta commit is AUTO
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

2 participants