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

custom watermark ID #1144

Closed
dacbd opened this issue Sep 8, 2022 · 8 comments · Fixed by #1173
Closed

custom watermark ID #1144

dacbd opened this issue Sep 8, 2022 · 8 comments · Fixed by #1173
Assignees
Labels
cml-comment Subcommand discussion Waiting for team decision enhancement New feature or request p1-important High priority

Comments

@dacbd
Copy link
Contributor

dacbd commented Sep 8, 2022

So that when cml goes to update a comment it can determine which comment to update.

For cases where multiple comments are made or when multiple workflows run against a single PR each with a cml comment, they can better distinguish which comment to update.

@dacbd dacbd added the cml-comment Subcommand label Sep 8, 2022
@casperdcl casperdcl added the enhancement New feature or request label Sep 8, 2022
@0x2b3bfa0
Copy link
Member

Potential duplicate of / related to #1076?

@dacbd
Copy link
Contributor Author

dacbd commented Sep 8, 2022

related but different enough

@tasdomas tasdomas self-assigned this Sep 9, 2022
@tasdomas
Copy link
Contributor

Ok, so if two different workflows call cml comment create --update, we'd want them to update the comments these workflows originally created, right?

What if the same github workflow is run multiple times, only ever calling cml comment create --update - should it be updating the same comment?

I think we should avoid giving users too much rope (in the form of command flags) and just define several use cases we support.

@0x2b3bfa0
Copy link
Member

if two different workflows call cml comment create --update, we'd want them to update the comments these workflows originally created, right?

Yes, that's the goal of this issue.

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Sep 12, 2022

What if the same github workflow is run multiple times, only ever calling cml comment create --update - should it be updating the same comment?

That's what it currently does, but this use case is unavoidably prone to race conditions.

@tasdomas
Copy link
Contributor

It could be a lot simpler and less race-prone if we limited comment update to updating comments created by this action run only. Would that be too limiting?

@dacbd
Copy link
Contributor Author

dacbd commented Sep 12, 2022

It could be a lot simpler and less race-prone if we limited comment update to updating comments created by this action run only. Would that be too limiting?

If there were multiple comment create's in a single workflow this would make sense. Where this causes issues is with pull_request type events. Where there might be several runs of a workflow for a single PR either via multiple workflows or the user pushes additional commits retriggering the event. For this case in order to prevent extra bash scripting a single create_or_update type function is needed.

@DavidGOrtega
Copy link
Contributor

How different s going to be the wartermark? Studio needs the CML waterrmarks to be able to extract the CML report.

@DavidGOrtega DavidGOrtega added the discussion Waiting for team decision label Sep 13, 2022
@tasdomas tasdomas removed their assignment Sep 13, 2022
@casperdcl casperdcl linked a pull request Sep 20, 2022 that will close this issue
@tasdomas tasdomas self-assigned this Sep 20, 2022
@casperdcl casperdcl changed the title [send-]comment option for custom watermark custom watermark in comments Sep 22, 2022
@casperdcl casperdcl changed the title custom watermark in comments custom watermark ID Sep 26, 2022
@casperdcl casperdcl added the p1-important High priority label Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cml-comment Subcommand discussion Waiting for team decision enhancement New feature or request p1-important High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants