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

Version selection: add AWS redirect configs for latest docs version #561

Merged
merged 10 commits into from
Apr 6, 2022

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Dec 10, 2021

This also moves the generation of the versions file to the other push event file.

@netlify
Copy link

netlify bot commented Dec 10, 2021

Deploy Preview for crystal-book ready!

Name Link
🔨 Latest commit 9dd1cd3
🔍 Latest deploy log https://app.netlify.com/sites/crystal-book/deploys/624d5925a9ac9e0008e51140
😎 Deploy Preview https://deploy-preview-561--crystal-book.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

branches:
- '[0-9]+.[0-9]+'
- create
- delete
Copy link
Member Author

Choose a reason for hiding this comment

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

Hm is it possible to limit the range of branches that trigger this?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, no. These events are global and unfiltered.

I suppose we could filter for version branches in the steps. I think ${ github.event.ref } should hold the name of the branch (or tag) that was created or deleted.
Per https://docs.github.com/en/actions/learn-github-actions/contexts and https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#create

Maybe it would also be a viable alternative to not automate this in this repo, but integrate the steps with the release workflow over in https://github.com/crystal-lang/distribution-scripts (see https://github.com/crystal-lang/distribution-scripts/blob/master/processes/crystal-release.md)

Copy link
Member

Choose a reason for hiding this comment

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

The addition of a new branch (like when we release 1.3) needs to be triggered somehow. At that point we can also update the versions and website configuration.

Deletion is probably not even worth automating. That should never happen and if it does, it's because of manual interference.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this again, I think we should have all the configuration-level changes manually triggered (maybe it can be automatically triggered as part of the release process at some point, but that's not super important).

The workflow could go like this:

  1. Checkout https://github.com/crystal-lang/crystal-book
  2. Create a new version branch from master and push it
  3. Wait for deployment
  4. Update latest redirects (S3 bucket website configuration) and versions.json

https://github.com/crystal-lang/distribution-scripts/ seems like a good place for that because we have everything else already collected there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see much advantage to making it manually triggered (The creation of a branch in this repo is already a manual action BTW), but doesn't matter much to me. I agree with your idea.

This part is not critical for the moment, but clearly the way to proceed is to make a PR on distribution-scripts with these same scripts but slightly adapted. I am personally not interested in contributing to distribution-scripts but let me know if you think it's best that I do that.

And out of this PR only the change to .github/workflows/deploy-book.yml needs to be kept. But that should be applied after the other side (distribution-scripts) is finalized.

run: scripts/docs-versions.sh origin | tee /dev/stderr > versions.json
- name: Build aws-config.json
run: |
latest_version=$(git ls-remote --heads origin | grep -P -o '(?<=refs/heads/)[0-9][0-9.]+' | sort -V -r | head -n1)
Copy link
Member Author

Choose a reason for hiding this comment

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

So now we have this git ls-remote code in two places. I don't understand what the trouble was to keep this code snippet within the script itself. Can I change it back?

Copy link
Member

Choose a reason for hiding this comment

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

It's different concerns. I'd like that to be separate processes. The current implementation closely resembles the setup we're already using for the API docs over at https://github.com/crystal-lang/distribution-scripts/blob/8328ae59292c05cf8c830a34b5b39ac46c43813e/docs/Makefile#L43-L55
I'm sure we can remove the duplicate invocation by caching the result in an output variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not different concerns, though. It is exactly the same concern - "make versions up to date", just that AWS happens to handle one side of it separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider that both sides of this operation mainly change the meaning of "latest" - one for MkDocs and one for AWS. They shouldn't be separated.

Copy link
Member Author

Choose a reason for hiding this comment

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

With this in mind, I added back some of my refactors. Sorry for the trouble and I realize it's kinda rude.
We can definitely keep discussing this and change back still.

scripts/aws-config.json Outdated Show resolved Hide resolved
@oprypin oprypin marked this pull request as draft December 16, 2021 23:42
@straight-shoota
Copy link
Member

I'm coming back to this and I think it's probably better to keep everything here instead of moving parts of the deployment configuration over to https://github.com/crystal-lang/distribution-scripts/ It's nice to have everything contained here and we already have the automatic deployment here anyway.

An alternative to triggering deploy-config on every branch creation could be to trigger it only manually (workflow_dispatch) and then it could also take care of creating the release branch (via an input parameter).
But not sure if that's much of an improvement.

The main concern about the create trigger is, that it runs whenever any branch is created on this repo. But that's not actually a big issue. It's just a no-op. And we don't usually have non-release branches on this repo, so it's not happening that much.

Release branches are usually not deleted, so we can ignore that.
In the odd case, the workflow can be triggered manually.
@straight-shoota straight-shoota marked this pull request as ready for review April 6, 2022 11:15
@straight-shoota straight-shoota merged commit f79332d into crystal-lang:master Apr 6, 2022
@straight-shoota
Copy link
Member

straight-shoota commented Apr 6, 2022

This workflow ran for deployment of the release/1.4 branch, and it's successful.

A problem is there though: Deploy config finishes before Deploy book and in the time between, the redirect is already in place but the target (at /reference/1.4) is not yet available. So we essentially need a dependency here. Probably means we should merge them back into one workflow.
If this means running the deploy config on every build, that's probably okay. Maybe we can still find a way to run it only when a new release branch is created (for example, check with S3 if the release folder already exists before pushing it).

See #605

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.

3 participants