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

actions/pr-deployment: Fix deployment URL #19

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

teodutu
Copy link

@teodutu teodutu commented Oct 30, 2023

The URL was http://open-education-hub.github.io/.... This was incorrect for forked repos as it referred to the original owner: OEH. In addition, it was using the http scheme instead of https. This was less of an issue because the http endpoint redirects to https.

This commit fixes both of the aforementioned problems by dynamically obtaining the owner uwing ${{ github.repository_owner }} and by using https instead of http.

@teodutu teodutu added area/infra Update to infrastructure / scripts kind/fix Fix issues with existing content / item labels Oct 30, 2023
@teodutu teodutu requested a review from Alex-deVis October 30, 2023 12:52
@teodutu teodutu self-assigned this Oct 30, 2023
@teodutu teodutu added the needs-rendering The PR makes changes to the website that need to be rendered label Oct 30, 2023
@github-actions
Copy link

@Alex-deVis
Copy link

Published at http://open-education-hub.github.io/operating-systems/19/

Shouldn't this PR affect the running action too?

@teodutu
Copy link
Author

teodutu commented Oct 30, 2023

Published at http://open-education-hub.github.io/operating-systems/19/

Shouldn't this PR affect the running action too?

@gabrielmocanu, you know how this fucked up shit works. Should it?

@Alex-deVis
Copy link

@teodutu, check the workflow file [1]. It shows that your change is applied, but it does not have the expected result.

[1] https://github.com/cs-pub-ro/operating-systems/actions/runs/6693059404/workflow?pr=19

The URL was `http://open-education-hub.github.io/...`. This was incorrect
for forked repos as it referred to the original owner: OEH. In addition,
it was using the `http` scheme instead of `https`. This was less of an
issue because the `http` endpoint redirects to `https`.

This commit fixes both of the aforementioned problems by dynamically
obtaining the owner uwing `${{ github.repository_owner }}` and by using
`https` instead of `http`.

Signed-off-by: Teodor Dutu <[email protected]>
@teodutu teodutu force-pushed the fix-pr-deployment-url branch from 38fbc44 to d4c8289 Compare October 30, 2023 13:06
@teodutu
Copy link
Author

teodutu commented Oct 30, 2023

You're right. I'll investigate.

@gabrielmocanu gabrielmocanu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 30, 2023
@github-actions
Copy link

@gabrielmocanu
Copy link
Member

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

@gabrielmocanu gabrielmocanu self-requested a review October 30, 2023 15:09
@gabrielmocanu gabrielmocanu merged commit c25c91f into cs-pub-ro:main Oct 30, 2023
3 of 4 checks passed
@gabrielmocanu
Copy link
Member

@teodutu no need for investigation, it works.
Check this out: #17 (comment)

@gabrielmocanu
Copy link
Member

Here to save your day, as always.

@teodutu teodutu deleted the fix-pr-deployment-url branch November 28, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra Update to infrastructure / scripts kind/fix Fix issues with existing content / item needs-rendering The PR makes changes to the website that need to be rendered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants