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

Update #2

Closed
wants to merge 21 commits into from
Closed

Update #2

wants to merge 21 commits into from

Conversation

deemp
Copy link
Contributor

@deemp deemp commented Jul 12, 2023

Hi, @Arjan-Zuidema !

I updated the action a bit:

  • Switched to TypeScript
  • Added functionality to remove caches from multiple pages

@Arjan-Zuidema
Copy link
Contributor

Hi @deemp!

Thanks for your pull request! Looks good for the most part. I added some comments, if you can reply/fix those we can merge it 🙂

@deemp
Copy link
Contributor Author

deemp commented Jul 12, 2023

@Arjan-Zuidema, unfortunately, I don't see your comments

@Arjan-Zuidema
Copy link
Contributor

Just scroll up in this window 🙂

@deemp
Copy link
Contributor Author

deemp commented Jul 12, 2023

Here's what I see :)
image

@Arjan-Zuidema
Copy link
Contributor

Check files changed tab

@deemp
Copy link
Contributor Author

deemp commented Jul 12, 2023

This is how comments should look like

package.json Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@Arjan-Zuidema
Copy link
Contributor

@deemp I added mentions and request changes

@Arjan-Zuidema
Copy link
Contributor

@deemp What is NODE_OPTIONS=--openssl-legacy-provider used for?

@deemp
Copy link
Contributor Author

deemp commented Jul 14, 2023

@Arjan-Zuidema, I answered in a review. Is it visible?

image

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@deemp
Copy link
Contributor Author

deemp commented Jul 14, 2023

My bad, I didn't submit

package.json Outdated Show resolved Hide resolved
@deemp deemp force-pushed the master branch 4 times, most recently from a31252c to 6fdca5e Compare July 15, 2023 10:11
@deemp
Copy link
Contributor Author

deemp commented Jul 15, 2023

I updated the workflow to use it as an example

@deemp
Copy link
Contributor Author

deemp commented Jul 15, 2023

As there are a lot of commits, I guess it'd be better to squash them before merging

@deemp
Copy link
Contributor Author

deemp commented Jul 16, 2023

@Arjan-Zuidema , please, have a look

Copy link
Contributor

@Arjan-Zuidema Arjan-Zuidema 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! Just added one more thing, if you can fix that we can merge it :)

This reverts commit 34a52af.
@deemp
Copy link
Contributor Author

deemp commented Jul 18, 2023

Workflows in my fork fail. Will look into it later

@deemp
Copy link
Contributor Author

deemp commented Jul 18, 2023

@Arjan-Zuidema , what can be the cause?
A cache can't be deleted.

https://github.com/deemp/purge-cache/actions/runs/5587233430/jobs/10212852447#step:3:23

@Arjan-Zuidema
Copy link
Contributor

Seems like a permission issue. But I think that should not happen on a GitHub action run. I think this case needs to be handled so the rest of the run will continue. Could you add that in the PR? Just wrap it with a try catch and log the error

@Arjan-Zuidema
Copy link
Contributor

This is merged in #3 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants