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

Tutorial Updates #1832

Merged
merged 288 commits into from
Feb 29, 2024
Merged

Tutorial Updates #1832

merged 288 commits into from
Feb 29, 2024

Conversation

github-actions[bot]
Copy link
Contributor

This is an automated pull request to let you know there are new content available for the tutorial!

If these changes look good to you, it is recommended that you merge the PR using the Squash and merge feature.

If there are any issues with the content here, do not edit these files directly – the original source lives in the super-rentals-tutorial repo.

Feel free to hold off on merging this PR and file an issue/PR in the upstream repo. After fixing the issues there, the upstream CI job will push the changes to this pull request branch, automatically, so you should see the changes reflected in this PR shortly after.

That being said, it's totally okay to push other changes to the super-rentals-tutorial branch manually, like making changes to the TOC or adding words to the dictionary. Just don't edit the markdown files themselves, since any changes you make will be wiped out by the next upstream CI job.

You can rebase the branch and force-push, or even delete it! The CI job clones the branch fresh on each build, and re-creates it from the master branch if needed, so there shouldn't be any issues with conflicts and such.

@ef4
Copy link
Contributor

ef4 commented Jan 26, 2024

Hi @ember-learn/learning-core-team, we have two years of improvements to the tutorials that haven't been published because this PR is lingering. How can we fix the process here?

@chancancode
Copy link
Contributor

When I wrote the message in the automated PR, I probably made it sound too scary?? It’s not actually a very complicated process if you understand what’s up.

The original sins are:

  1. we are didn’t invest in a good set up to store images separately than git, and using git for binary storage is not great/efficient, and they will be versioned forever

  2. despite my best attempt at the time, the automated builds still occasionally (too often) spit out subtly different images. There is a step to filter these out, but sometimes that is out of our control, like mapbox tweaking the thickness of the lines, sometimes chrome render things 1px off

So the combination of those two make it slightly less straightforward than just clicking the merge button, but not that much more

As far as how to merge this PR, the key is we don’t want to merge in the image diffs at all if they aren’t meaningfully different, and if we do want to merge we want to make sure to merge only those we want to keep and squash, so that we are only adding the minimum amount of binary bloat to git.

Mechanically, that usually means to pull the branch, discard the unwanted diffs by checking out the version from main, then either squash locally or using the squash to merge button.

It takes a few minutes on a computer once you are fluent with git.

As far as how we can fix this process wise:

  1. we can solve the underlying issue of storing binary in tree, maybe with git LFS or upload them separately to a CDN, or

  2. we can invest in hacking the build to ensure the screenshots don’t accidentally change, by vendoring the fonts and map images locally, pinning the version of chrome, or

  3. stop sending any image diffs over here at all, and require manual work when/if we do want to update them

All of them are perfectly doable, I was going to do 3 but didn’t get around to do it. It’s not ideal, but given that we didn’t intentionally update the tutorial content that often it may be fine in practice.

Happy to pair on either doing the merge or that tweak to the automation. Ideally I don’t just fix this myself and no one learns to do it

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

I'm requesting changes on this because we are in the process of fixing this issue and this PR should not be merged as is

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

We have fixed all the previous versions of the tutorial with #2000 and we need to now merge this PR to fix the current release version. This will tide us over until the next release (6 weeks from now i.e. 5.8.0) to properly integrate the tutorial updates into our release process 👍

@mansona
Copy link
Member

mansona commented Feb 29, 2024

Usually I would squash merge this massive PR, but our process for fixing the previous versions of the tutorial made good use of the history in this branch so I am going to create a merge commit to retain history 👍

@mansona mansona merged commit 598eb9a into master Feb 29, 2024
6 checks passed
@mansona mansona deleted the super-rentals-tutorial branch February 29, 2024 16:29
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.

4 participants