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

Fix delete preview comment domain #125

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Nov 15, 2024

This pull request includes a minor change to the .github/workflows/preview-cleanup.yml file. The change updates the URL format in the message for deleting a preview when a pull request is closed.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I doubt it will work either, because we won't have a location on the site after deletion. I suggest just referencing the GitHub repo for now, and there we will need some placeholders for deleted patches

@@ -33,4 +33,4 @@ jobs:
number: ${{ github.event.number }}
id: deploy-preview
message: >
🪓 PR closed, deleted preview at https://github.com/${{ env.PREVIEW_REPO }}/tree/gh-pages/${{ env.PPREVIEW_PATH }}/
🪓 PR closed, deleted preview at https://${{ env.PREVIEW_DOMAIN}}/${{ env.PREVIEW_REPO }}/tree/gh-pages/${{ env.PPREVIEW_PATH }}/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
🪓 PR closed, deleted preview at https://${{ env.PREVIEW_DOMAIN}}/${{ env.PREVIEW_REPO }}/tree/gh-pages/${{ env.PPREVIEW_PATH }}/
🪓 PR closed, deleted preview at https://github.com/gradle/${{ env.PREVIEW_REPO }}/tree/gh-pages/${{ env.PREVIEW_PATH }}/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I got the whole thing 😅
Sorry.

However, why can't we use the same thing on as we do on ci? 🤔
https://${{ env.PREVIEW_DOMAIN }}/${{ env.PREVIEW_REPO }}/${{ env.PREVIEW_PATH}}/
This is actually what I would expect...

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 15, 2024 via email

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 15, 2024 via email

@StefMa
Copy link
Contributor Author

StefMa commented Nov 17, 2024

The directory is deleted after the merge, right? So the content is also gone? This means the link currently leads to a non-existent directory, correct? 🤔

Having a link to a non-existent directory doesn’t make sense. I thought my fix was meant to indicate, "This deployment was deleted, so it's no longer available." Well, I’m not sure if that helps, but at least it matches the initial GitHub Actions comment saying, "It was deployed to."

That said, I agree with you that maybe it would be better to omit the link altogether. Why say something like, "It was there, but not anymore"? It doesn’t provide any useful information.

Perhaps a better approach would be to check if the PR was merged. If it was, we could say, "Deployment deleted; content is available at [Main URL]." If the PR was closed without merging, we could say, "Preview deleted."

I’ll update my PR with that tomorrow. 🙃

@StefMa
Copy link
Contributor Author

StefMa commented Nov 17, 2024

Code update ✅

@oleg-nenashev
Copy link
Member

Thank you !

@StefMa
Copy link
Contributor Author

StefMa commented Nov 17, 2024

CI seems to be failing because it uses secrets.GITHUB_TOKEN under the hood. Forks doesn't have access to it 😔

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 17, 2024 via email

@oleg-nenashev
Copy link
Member

Raised #128 , will merge it for now as it should work

@oleg-nenashev oleg-nenashev merged commit 41490ad into gradle:main Nov 18, 2024
1 check failed
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.

2 participants