Fix archive for non-existent labels #14

emcfarlane · 2024-06-04T20:40:07Z

This PR fixes two issues with the archive step. First is a typo in the default event name of archive_labels. This should be github.event.ref not github.event.ref_name. The other fix is for more robust handling of multiple non existent labels. For delete events we cannot guarantee the branch or tag is matched to a label in the BSR. We now iterate over all labels and handle missing failures. On a label not existing the archive command will log the failed output but still succeed.

github-actions · 2024-06-04T20:41:05Z

The latest Buf updates on your PR.

Name	Status
build	✅ passed
lint	⏩ skipped
format	❌ failed
breaking	✅ passed

nicksnyder

How did we figure out this was an issue?

nicksnyder · 2024-06-05T00:59:15Z

src/main.ts

@@ -279,7 +279,14 @@ async function archive(bufPath: string, inputs: Inputs): Promise<Result> {
  for (const label of inputs.archive_labels) {
    args.push("--label", label);
  }
-  return run(bufPath, args);
+  const result = await run(bufPath, args);


How does the backend behave if we try to archive multiple labels at once and one of the labels doesn't exist. Does the whole RPC atomically fail? If so, I think we want to best effort archive as many labels as possible so I think we might want to make one archive command per label that needs to be archived.

Whats the use case for one to succeed?

not sure I understand your question, can chat tomorrow if you want

If one of the labels doesn't exist it will fail atomically. The default, and the most common case, would be to archive the one label from the delete event. So if multiple tags got deleted at once each event would fire and archive them in separate CI events. If a user overrides and wants multiple events I think failing if one doesn't exist it is correct to fail and we shouldn't try to run them one by one. Happy to chat through.

Ok, so the difference here only matters if the user configured the GHA with one or more extra labels for archiving. Thinking about it now, why do we allow them to configure extra labels for archiving? Not sure that actually makes sense.

We allow configuring more than one label for push, so makes sense if they have a multiple label pattern.

We allow configuring more than one label for push, so makes sense if they have a multiple label pattern.

Ok, yeah makes sense.

If a user overrides and wants multiple events I think failing if one doesn't exist it is correct to fail and we shouldn't try to run them one by one.

Can chat about this live today, but I guess I view archiving state in the BSR as a cleanup task. If the archive task has been configured with let's say 2 labels, and one is valid but the other isn't, wouldn't you as a user prefer that the valid one gets archived even thought the other one doesn't exist?

Whether to mark the job as failed or not if this happens is a separate question, but I would be fine not marking the job has failed because we have asserted some state (i.e. the labels in question have been archived or don't exist)

Passing-by comment: All APIs in v1/v1beta1 are atomic in its implementation, either all of them succeed or all of them fail, no partial success.

It's not explicitly documented in the ArchiveLabels RPC, but it would be weird having some RPCs that support partial success and others that are atomic. If we go down the path of partial success, we'd probably need a way to communicate in the response which ones were archived and which ones were not found, and it would open the gate to allow partial success for other RPCs like Get* or Delete*.

I think another approach, probably much more cumbersome from the client side, but according to the API requirements, is to loop one by one doing Get -> Exists? -> Archive. That way even if some of the labels exist and some don't, the job doesn't fail.

Correct, I was not suggesting changing the API, I am suggesting changing the implementation here to loop and make multiple calls.

emcfarlane · 2024-06-05T01:39:54Z

This is an issue from testing. The current bug from #10 is currently obscuring it. We would noticed this for a CI setup with events filtered on push, so not every label is pushed but every delete triggers an archive:

on:
  push:
    paths: 'protos/**'
  delete:

Deletes aren't filterable, so we are required to handle the 404 case either in the action or as part of the CLI.

emcfarlane · 2024-06-06T18:03:38Z

Tested both the new default archive label name and handling of non-existent errors.

/opt/hostedtoolcache/buf/1.32.2/x64/buf beta registry archive --label ed/notPushed
Failure: label with name "***/library:ed/notPushed" was not found
Skipping archive, label ed/notPushed not found

nicksnyder · 2024-06-06T18:39:12Z

Does this implementation fail the job or not fail the job if one archive fails but others succeed?

emcfarlane · 2024-06-06T18:46:48Z

@nicksnyder does not fail the job if one fails for non-exist. Reports the failure in the logs of CI.

nicksnyder · 2024-06-06T18:48:03Z

Great, thanks for confirming!

Fix archive on label not found

0561162

emcfarlane marked this pull request as ready for review June 4, 2024 20:41

emcfarlane requested review from nicksnyder, doriable and smallsamantha June 4, 2024 20:41

nicksnyder reviewed Jun 5, 2024

View reviewed changes

emcfarlane added 4 commits June 6, 2024 13:18

Merge branch 'main' into ed/allowArchiveFailure

712abaf

Fix default delete event ref

2f6560c

Handle partial failures on multiple

8ad5748

Make pass event

2741679

emcfarlane changed the title ~~Fix archive on label not found~~ Fix archive for non-existent labels Jun 6, 2024

doriable approved these changes Jun 6, 2024

View reviewed changes

emcfarlane merged commit 8c610cc into main Jun 6, 2024
29 checks passed

emcfarlane deleted the ed/allowArchiveFailure branch June 6, 2024 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix archive for non-existent labels #14

Fix archive for non-existent labels #14

emcfarlane commented Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

nicksnyder left a comment

nicksnyder Jun 5, 2024

emcfarlane Jun 5, 2024 •

edited

Loading

nicksnyder Jun 5, 2024

emcfarlane Jun 5, 2024 •

edited

Loading

nicksnyder Jun 5, 2024

emcfarlane Jun 5, 2024

nicksnyder Jun 6, 2024

unmultimedio Jun 6, 2024 •

edited

Loading

nicksnyder Jun 6, 2024

emcfarlane commented Jun 5, 2024 •

edited

Loading

emcfarlane commented Jun 6, 2024 •

edited

Loading

nicksnyder commented Jun 6, 2024

emcfarlane commented Jun 6, 2024

nicksnyder commented Jun 6, 2024

Fix archive for non-existent labels #14

Fix archive for non-existent labels #14

Conversation

emcfarlane commented Jun 4, 2024 • edited Loading

github-actions bot commented Jun 4, 2024 • edited Loading

nicksnyder left a comment

Choose a reason for hiding this comment

nicksnyder Jun 5, 2024

Choose a reason for hiding this comment

emcfarlane Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

nicksnyder Jun 5, 2024

Choose a reason for hiding this comment

emcfarlane Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

nicksnyder Jun 5, 2024

Choose a reason for hiding this comment

emcfarlane Jun 5, 2024

Choose a reason for hiding this comment

nicksnyder Jun 6, 2024

Choose a reason for hiding this comment

unmultimedio Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

nicksnyder Jun 6, 2024

Choose a reason for hiding this comment

emcfarlane commented Jun 5, 2024 • edited Loading

emcfarlane commented Jun 6, 2024 • edited Loading

nicksnyder commented Jun 6, 2024

emcfarlane commented Jun 6, 2024

nicksnyder commented Jun 6, 2024

emcfarlane commented Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

emcfarlane Jun 5, 2024 •

edited

Loading

emcfarlane Jun 5, 2024 •

edited

Loading

unmultimedio Jun 6, 2024 •

edited

Loading

emcfarlane commented Jun 5, 2024 •

edited

Loading

emcfarlane commented Jun 6, 2024 •

edited

Loading