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

Add action to push screenshots to conda store #346

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

kcpevey
Copy link
Contributor

@kcpevey kcpevey commented Dec 14, 2023

Fixes conda-incubator/conda-store#570

Description

This workflow collects screenshots which have stored as artifacts in test.yml and opens a PR on conda-store to update the docs.

This pull request:

  • adds gh action to push screenshots to conda-store

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

We originally discussed triggering this from a comment on a PR. I discovered that the only Action trigger, issue_comment, would trigger this to run for every comment on every issue. I feel like that is a lot of wasted compute even if the first thing it did was check to see if its a PR and shut it down. For this reason, I've decided to trigger on label instead.

# There are two ways to trigger this workflow:
# 1. Trigger by adding the `update_screenshots` label. 
#    The workflow will run immediately after you add the label. It is
#    also expecting artifacts which have been generated in test.yml. 
#    Therefore, this cannot be triggered until test.yml is complete. 
# 2. Trigger by workflow dispatch.
#    Specify the PR number with the artifacts you'd like to use.
#    Again, the test.yml workflow must be complete.
#
# If a PR has already been opened on the conda-store repo from this
# action, a rerun of this action will attempt to update the PR, not
# create a new one. 

I looked at triggering on the completion of test.yml, but that would trigger the workflow off of main and we'd be forced to use the "latest" generated artifacts, rather than PR-specific.

This will require a PAT to open up the PR on the conda-store side. It is currently looking for secrets.PAT.

@kcpevey
Copy link
Contributor Author

kcpevey commented Dec 14, 2023

I've tested this approach on dummy repos but this will still take me some tuning to get everything right in place.

TODO:

  • get secrets sorted out
  • add more screenshots to playwright to cover what's already in conda-store
    • make sure the words of the doc aren't specific to the old screenshot (i.e. has the doc been updated to the new UI)
  • sort out where these need to land (in the new docs folder or overwrite the old screenshots?)

@kcpevey
Copy link
Contributor Author

kcpevey commented Dec 14, 2023

@pavithraes it looks like a lot of the images in /docusaurus-docs/conda-store-ui/images have red boxes around the area of interest. We will lose those if we let playwright generate the images. Is that ok, or should we avoid updating those images with playwright?

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Just a few questions, but this looks close to the finish line!

.github/workflows/push_images.yml Outdated Show resolved Hide resolved
.github/workflows/push_images.yml Show resolved Hide resolved
.github/workflows/push_images.yml Show resolved Hide resolved
test/playwright/test_ux.py Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kcpevey
Copy link
Contributor Author

kcpevey commented Sep 16, 2024

Ok, this thing is back to passing CI 🎉 Now I need to double check the images.

@kcpevey
Copy link
Contributor Author

kcpevey commented Sep 16, 2024

I suspect this is failing due to a bad token. We were originally using @jaimergp 's PAT. Can you please regenerate and add it as a secret named CONDA_STORE_SYNC in this repo? It needs to be able to open a PR on https://github.com/conda-incubator/conda-store

@gabalafou
Copy link
Contributor

gabalafou commented Sep 17, 2024

I suspect this is failing due to a bad token. We were originally using @jaimergp 's PAT.

I have a question about this. Is this going to create any maintenance issues in the future? Is this the most future-proof approach we can take?

@kcpevey
Copy link
Contributor Author

kcpevey commented Sep 17, 2024

I have a question about this. Is this going to create any maintenance issues in the future? Is this the most future-proof approach we can take?

Because we are opening a PR on another repo, we need credentials. In other projects, we have github service accounts where we share credentials (so basically internal "bot" users). Since this would have to be a user in the conda-incubator org, we didn't feel that was appropriate and decided to go with personal credentials. Happy to explore any other ideas though!

Copy link

A PR to conda-store has been opened with updated screenshots - conda-incubator/conda-store#731

@jaimergp
Copy link
Member

Fixed.

@kcpevey
Copy link
Contributor Author

kcpevey commented Sep 19, 2024

I looked through the built docs on the other PR. I need to make a few minor tweaks to some of the image clipping.

Copy link

A PR to conda-store has been opened with updated screenshots - conda-incubator/conda-store#731

git config user.name ${{ github.actor }}
git config user.email '${{ github.actor }}@users.noreply.github.com'
git add -A
git commit -m "commit all changes"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still like to see this commit message updated

Suggested change
git commit -m "commit all changes"
git commit -m "🤖 conda-store-ui/update_screenshots"

Copy link
Contributor Author

@kcpevey kcpevey Sep 20, 2024

Choose a reason for hiding this comment

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

ah. Sorry, I changed this line instead when you requested the first time.

Ok... little bit of a wild goose chase, but I think I got it. The commit-message below (line 92) is being ignored and THIS line is what's being used.

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Yay, this is coming together. Sorry to insist, but I really want us to update the commit message in the workflow

@kcpevey
Copy link
Contributor Author

kcpevey commented Sep 20, 2024

Unfortunately I had the images looking great on my local setup (which is running the same conda-store version as CI) but the images I generated locally differ from CI. The images that require clipping are "off" by an unacceptable amount.

@gabalafou
Copy link
Contributor

Sorry to possibly rock the boat here, but I am starting to wonder if we should abandon this PR.

Similar to CI, I also had iffy results running this locally. For example, the following screenshot is not ready to be published straight to the docs:

add-channel.png

It's to be expected that there will be differences between the screenshots taken on CI and those taken locally. This came up when I started working on JupyterLab, which has a number of snapshot tests, which compares saved screenshots taken on a previous run and committed to the repo with screenshots taken on the current run. If you did anything that changed the UI, you couldn't update the saved and committed screenshots from your local computer; you had to upload your changes in a PR and then ask a GitHub robot to attach updated screenshots to your PR.

I don't want us to fall into a sunk cost fallacy here. We have spent a lot of time to get this PR close to the finish line, but considering the work left to do and the work it may demand in the future, have we already tipped past the value this PR potentially provides?

@trallard
Copy link
Collaborator

I am leaning towards the same view as @gabalafou, this PR has lagged a lot more than I would have liked both in time and effort, and I am worried that it will add a non-trivial amount of maintenance and upkeep work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In review 👀
Development

Successfully merging this pull request may close these issues.

[DOC] Add Playwright tests to grab relevant screenshots
5 participants