Checking attestors for duplicates #332
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,12 +85,22 @@ | |
| attestors = append(attestors, commandrun.New(commandrun.WithCommand(args), commandrun.WithTracing(ro.Tracing))) | ||
| } | ||
|
|
||
| for _, a := range ro.Attestations { | ||
| for _, att := range attestors { | ||
| if a == att.Name() { | ||
| log.Warnf("Attestator %s already declared, skipping", a) | ||
| break | ||
| } else { | ||
| attestor, err := attestation.AddAttestor(a) | ||
|
Check failure on line 94 in cmd/run.go
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to create attestor: %w", err) | ||
|
There was a problem hiding this comment. Do we want to fail at the first attestor, or would it be better to continue trying to add the subsequent attestors and give the user an error with all failed attestors? Example: Let's say the user sends attestors We will warn the user about Feel free to ignore if I'm missing some context here :) There was a problem hiding this comment. hmmm, so I don't feel really strongly either way, but I suppose you're asking 'if failing to create an attestor fails, should we fail?'. If the Going back to the original problem that this PR removes, I tackled this by warning the user that they have repeated themselves / declared an attestor that has already been declared (e.g., by default). Possibly it could be a better practice to just fail in this situation. After all, it may be better to be simple and say "you've duplicated attestor defintion, please fix the invocation". There was a problem hiding this comment. @jkjell @mikhailswift, any thoughts? There was a problem hiding this comment. To throw a 🔧 in the conversation, I create #340 to capture thoughts around a future idea that may be counter to this one. There was a problem hiding this comment. @jkjell I think that what you mention in #340 makes sense, but I don't think it collides too much with this due to the fact that (at least for now) there is no reason why you would want to run an attestor more than once. I think that we should merge this for now, and we could potentially change it in future if/when we implement #340. |
||
| } | ||
| attestors = append(attestors, attestor) | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, attestor := range attestors { | ||
| setters, ok := ro.AttestorOptSetters[attestor.Name()] | ||
| if !ok { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the
autobuildstep from here (as the comments instructed) as it is leading to behaviour that left the step indefinitely hanging (see here). In my opinion such exhaustive use of compute time should be avoided at all costs.I think it would be nice to get back to using the
autobuildscripting, however it does introduce some guess work and ambiguity that can be eradicated by simply pointing the workflow atmakeormake build. I also don't believe that this change sacrifices any functionality.@jkjell @kairoaraujo would be good to get opinions here. I also recognise that this PR may not be the place for such a change, if you want I can go ahead and move it to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this was from the job or because we're an OSS project but, the billable time was
0 sfor the failure: https://github.com/in-toto/witness/actions/runs/7251758704/usage. It looks like the only thing the autobuild did was call make. So, that makes (😂) me think we could hit the same issue with a direct make command too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, yeah, if we want this change ➕ to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see what you mean by us being an OSS project, still gives me the heebiejeebies.
As for what caused it I am still not totally sure, but the
autobuildlogic actually performsmake(which fails obviously) and continues anyway:Whatever the matter is, I feel it's more deterministic to do it this way, so I reckon we should split this out ot a separate PR regardless. I don't think we should trust any automation that we don't necessarily need? If it wants to know how we build, we can just tell it 😄 .