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

Fixes for Preview URL Redirects, CSS Asset Fetching, and Post-Merge Preview URL Availability #96

Merged
merged 7 commits into from
Aug 17, 2023

Conversation

cesrjimenez
Copy link
Contributor

@cesrjimenez cesrjimenez commented Aug 10, 2023

Summary:
This PR addresses several bugs associated with our URL handling and asset fetching:

  1. Redirect issues with preview URLs in local development.
  2. Assets not being updated upon changes to CSS.
  3. Non-functional preview URL post-merge.

Changes:

  1. Redirect Fixes:
  • Modified our URL resolution logic to handle both local and production preview URLs seamlessly.
  • Introduced a variable DEBUG to differentiate between local and production modes. The URL routing mechanism will now adapt based on this flag. It's only used for development.
  • Enhanced error handling to log any redirect failures for easier debugging in the future.
  1. Asset Fetching and CSS Change:
  • Discovered that our asset serving functionality was not updating the version changes.
  • Implemented a fix where CSS and images assets will now update correctly upon fetching a version, ensuring that the latest version of the asset is fetched and served.

Testing:

  • Make sure DEBUG variable is set to true in cmd/docsite/site.go line 174 to mimic production enviroment
  • In the cmd/docsite directory run go build .
  • Then in the sourcegraph repo, in the doc/serve.sh file change the $docsite variable to point to your local build of docsite. (my $docsite variable looks like this 👉 docsite_bin=/Users/<name>/go/src/docsite/cmd/docsite/docsite
  • Then in sourcegraph repo root, run sg run docsite
  • Go to the url http://localhost:5080/@mrn-docs-preview
  • Test preview URLs locally to ensure consistent and expected behavior.
  • I also added unit tests for our URL resolution logic to prevent regressions in the future.

Request for Review:
I would appreciate it if team members could pull this branch and validate these fixes in their respective environments. Any feedback or potential edge cases that haven't been covered would be immensely valuable.

Thanks for your time, and looking forward to the feedback!

@csells
Copy link

csells commented Aug 11, 2023

@Numbers88s I'm excited to see this new docs Preview in action. Can you make a PR with a docs change and add me and Maedah can see it?

@cesrjimenez cesrjimenez changed the title Fix preview Fixes for Preview URL Redirects, CSS Asset Fetching, and Post-Merge Preview URL Availability Aug 11, 2023
@cesrjimenez cesrjimenez marked this pull request as ready for review August 11, 2023 18:30
@cesrjimenez
Copy link
Contributor Author

@Numbers88s I'm excited to see this new docs Preview in action. Can you make a PR with a docs change and add me and Maedah can see it?

@csells Yes we can definitely do that, but not at the moment. We would need this PR merged on main first, since it updates the templates needed for this to work.

I can merge the work and then make a change and tag you both, if that works ✌️

@csells
Copy link

csells commented Aug 11, 2023

Perfect!

if err != nil {
return nil, err
}
return &subdirFileSystem{fs: c, path: "_resources/" + dir}, nil
Copy link

Choose a reason for hiding this comment

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

Might want to use filepath.Join here unless there's a reason unix filepaths are always alright (I think this is true for go-embed, for example).

@cesrjimenez cesrjimenez merged commit 65b3234 into main Aug 17, 2023
@cesrjimenez cesrjimenez deleted the fix-preview branch August 17, 2023 15:36
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