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

Upgrade hugo-bin-extended to 0.112.7 #1161

Closed
2 tasks
HussainTaj-arbisoft opened this issue May 24, 2023 · 6 comments · Fixed by #1167
Closed
2 tasks

Upgrade hugo-bin-extended to 0.112.7 #1161

HussainTaj-arbisoft opened this issue May 24, 2023 · 6 comments · Fixed by #1167
Assignees

Comments

@HussainTaj-arbisoft
Copy link
Contributor

HussainTaj-arbisoft commented May 24, 2023

Background / Purpose

We will upgrade ocw-course-publisher to 0.6 Hugo 0.112.7 (refer to mitodl/ocw-studio#1821), we also need to update hugo-bin-extended to 0.112.7 to match the version in Studio.

Description

Update hugo-bin-extended to 0.112.7 to match the version in Studio. This should be done in conjunction with or after mitodl/ocw-studio#1821.

Acceptance Criteria

  • Hugo is upgraded to 0.112.7
  • Dev environment works correctly
@ChristopherChudzicki
Copy link
Contributor

Suggestion: IMO, we should at least open the PR to upgrade hugo-bin-extended BEFORE doing the other work (studio PR, ocw-hugo-publisher update). Because:

  • updating hugo in ocw-hugo-themes is very low effort. We can open the PR, and wait to merge it.
  • our e2e tests will run

For example, the issue @gumaerc caught in mitodl/ocw-studio#1826 could (in principle) have been caught by a Playwright test in ocw-hugo-themes. It would not actually have been caught unless we do:

@gumaerc
Copy link
Collaborator

gumaerc commented Jun 2, 2023

For example, the issue @gumaerc caught in mitodl/ocw-studio#1826 could (in principle) have been caught by a Playwright test in ocw-hugo-themes.

This may be true, as we do run e2e tests with a baseURL, but I do think this is somewhat of a special case where a bug was introduced into Hugo itself and had nothing to do with our theme. Not that extra testing is ever bad, but when it comes to this kind of thing, where do we stop? Do we add tests anywhere we use .Permalink to make sure the link is as expected? Basically what I'm asking is: where do we draw the line between testing expected output based on how we are writing our theme vs tests that should just be part of Hugo itself?

Also, I think that at least in local dev when it comes to the hugo-bin-extended NPM package specifically, we should use syntax like "hugo-bin-extended": "~0.112.0", so that the latest patch version is automatically installed, rather than pinning a specific patch version. The Hugo team seems to put effort into making sure that major changes are limited to minor version updates, and that the patch versions are safe to automatically install. In this case, it would have allowed a patch version to automatically do its job, patching a bug in the minor version release.

@ChristopherChudzicki
Copy link
Contributor

ChristopherChudzicki commented Jun 2, 2023

All our tests assume that the root of the site is /

This is not accurate for our e2e tests:

Now serving the following sites:
┌────────────────────┬──────────────────────────────────────────────────┐
│ Site               │ URL                                              │
├────────────────────┼──────────────────────────────────────────────────┤
│ ocw-ci-test-course │ http://localhost:3010/courses/ocw-ci-test-course │
├────────────────────┼──────────────────────────────────────────────────┤
│ ocw-ci-test-www    │ http://localhost:3010/                           │
└────────────────────┴──────────────────────────────────────────────────┘

Roughly, the e2e tests:

  1. Build several hugo site (hugo, not hugo serve) to a directory:
    tmp
    |-- courses/
        |-- ocw-ci-test-course
        |-- ... *other potential courses could be added*
    |-- ocw-ci-test-www
    
  2. Serve the tmp/ directory with a static file server and redirect requests not beginning with "courses" to ocw-ci-test-www.

@gumaerc
Copy link
Collaborator

gumaerc commented Jun 2, 2023

Whoops, I edited my comment above after I realized my error, but for posterity I did make a comment assuming that we don't test with an explicit baseURL.

@HussainTaj-arbisoft
Copy link
Contributor Author

... we should at least open the PR to upgrade hugo-bin-extended BEFORE doing the other work

This makes sense to me. Thank you for the suggestion.

... we should use syntax like "hugo-bin-extended": "~0.112.0",

@gumaerc could setting "hugo-bin-extended": "~0.112.0" cause any inconsistent behavior between Studio and local builds? I'm inclined to keep the version pinned to exactly match the Studio's Hugo version. It should avoid a scenario like "It works on my machine but not the server, I don't know why." Although, I don't know how likely that scenario is in our project.

Since I was going to work on the publisher image today, I'll follow @ChristopherChudzicki's suggestion and test 0.112.7 on themes first and put up a draft PR.

@HussainTaj-arbisoft HussainTaj-arbisoft self-assigned this Jun 5, 2023
@HussainTaj-arbisoft HussainTaj-arbisoft changed the title Upgrade hugo-bin-extended to 0.112.0 Upgrade hugo-bin-extended to 0.112.7 Jun 5, 2023
@gumaerc
Copy link
Collaborator

gumaerc commented Jun 5, 2023

@gumaerc could setting "hugo-bin-extended": "~0.112.0" cause any inconsistent behavior between Studio and local builds? I'm inclined to keep the version pinned to exactly match the Studio's Hugo version. It should avoid a scenario like "It works on my machine but not the server, I don't know why." Although, I don't know how likely that scenario is in our project.

This is worth discussing, but I think that behavior is actually desirable. The locally installed node version would be our "canary in the coal mine" so to speak, giving us an indication of issues before they actually become a problem. Either way, if you're seeing weirdness it's not that hard to just change the string temporarily to pin the same version as studio and install that, to see if the behavior changes.

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

Successfully merging a pull request may close this issue.

3 participants