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.

Sign up for GitHub

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

Jump to bottom

Publish includes sha hash #1304

Closed
wants to merge 12 commits into from
Closed

Publish includes sha hash #1304

wants to merge 12 commits into from

Conversation

dacbd
Copy link
Contributor

@dacbd dacbd commented Jan 3, 2023 •
edited by 0x2b3bfa0
Loading

@dacbd dacbd temporarily deployed to internal January 3, 2023 23:51 — with GitHub Actions Inactive
@dacbd dacbd requested a review from a team January 3, 2023 23:52
@dacbd dacbd self-assigned this Jan 3, 2023
@dacbd dacbd added the cml-publish Subcommand label Jan 3, 2023
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal January 4, 2023 00:03 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
0x2b3bfa0
0x2b3bfa0 previously approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it's still pending resolution of https://github.com/iterative/studio/pull/4808#discussion_r1061251480 to determine whether to use the fragment or the query to store commit hashes.

casperdcl
casperdcl reviewed Jan 6, 2023
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

erm... don't we need to update genWatermark instead?

cml/src/cml.js

Line 206 in 506e4e8

return `![](${WATERMARK_IMAGE} "${title}")`;

@casperdcl casperdcl requested a review from 0x2b3bfa0 January 6, 2023 18:02
tasdomas
tasdomas suggested changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@tasdomas tasdomas left a comment

Choose a reason for hiding this comment

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

I don't think this will solve our issue with the studio integration.

We need to augment WATERMARK_IMAGE with a query fragment indicating the commit sha in genWatermark, but then the tricky bit is updating the logic we use to determine updatable comments here and here

src/cml.js Show resolved Hide resolved
@dacbd dacbd temporarily deployed to internal January 11, 2023 01:14 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal January 11, 2023 01:22 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal January 11, 2023 02:02 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal January 11, 2023 03:21 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd requested review from tasdomas and casperdcl January 11, 2023 05:21
@dacbd dacbd temporarily deployed to internal January 11, 2023 05:22 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd
Copy link
Contributor Author

dacbd commented Jan 11, 2023

I tested here to verify the changes work, but feel free to re-test in another way. iterative/cml-playground#286

@dacbd dacbd temporarily deployed to internal January 11, 2023 05:27 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal January 11, 2023 05:44 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas mentioned this pull request Jan 11, 2023
Merged
@dacbd dacbd closed this Jan 12, 2023
@dacbd dacbd deleted the publish-sha-hash branch January 12, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x2b3bfa0 0x2b3bfa0 Awaiting requested review from 0x2b3bfa0

@tasdomas tasdomas Awaiting requested review from tasdomas

@casperdcl casperdcl Awaiting requested review from casperdcl

Assignees

@tasdomas tasdomas

@dacbd dacbd

Labels
cml-publish Subcommand
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dacbd @tasdomas @casperdcl @0x2b3bfa0