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

Add app-content-pages dev server with devcert #4004

Merged
merged 7 commits into from Jan 9, 2023

Conversation

mcbouslog
Copy link
Contributor

@mcbouslog mcbouslog commented Dec 13, 2022

Package

  • app-content-pages

Why and Context

Describe your changes

  • adds dev server with devcert, dev server ran with yarn dev
  • update README accordingly ^

How to Review

Helpful explanations that will make your reviewer happy:

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

New Feature

  • The PR creator has listed user actions to use when testing the new feature

Refactoring

  • The PR creator has described the reason for refactoring
  • The refactored component(s) continue to work as expected

@mcbouslog mcbouslog added the dx Things that improve the developer experience label Dec 13, 2022
@mcbouslog mcbouslog requested a review from a team December 13, 2022 17:51
@mcbouslog
Copy link
Contributor Author

Noticed the app-project package.json has "devcert": "1.2.2" (no tilda). I'm not sure if that's intentional and why preferred to how this adds devcert with "devcert": "~1.2.2" (with tilda).

@coveralls
Copy link

coveralls commented Dec 13, 2022

Coverage Status

Coverage: 82.409% (+0.007%) from 82.402% when pulling fc72686 on app-content-pages-dev-server into 5944a17 on master.

@eatyourgreens
Copy link
Contributor

Devcert isn't well maintained nowadays, so I'm not sure how much to trust it.

If you do use it to generate a local certificate, I'd personally recommend removing the local CA that it installs, just for the peace of mind of knowing that your browser isn't using an untrusted CA to validate certifcates.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Dec 16, 2022

devcert was pinned to a specific version in #3118, to avoid a bug in 1.2.1, and never changed back.

@mcbouslog
Copy link
Contributor Author

Dependabot bumped it to 1.2.2 in #3297

@goplayoutside3 goplayoutside3 self-assigned this Jan 5, 2023
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

Looks good! I'm able to login with my account for staging while running app-content-pages locally. Noting that this will not work if running locally on a mobile device (and it's not expected to) due to the CORS error noted in the Readme.

@github-actions github-actions bot added the approved This PR is approved for merging label Jan 9, 2023
@eatyourgreens
Copy link
Contributor

I'd vote for removing devcert completely, since it isn't well maintained any more and it bypasses the browser's untrusted certificate warning (unless you remove the CA that it installs.)

@goplayoutside3
Copy link
Contributor

@eatyourgreens if devcert is removed from both app-project and app-content-pages, what is your suggested solution to enabling sign-in while developing locally?

@mcbouslog
Copy link
Contributor Author

I'd vote for removing devcert completely, since it isn't well maintained any more and it bypasses the browser's untrusted certificate warning (unless you remove the CA that it installs.)

@eatyourgreens - is there an alternative to devcert or a different way to run the FEM apps locally with Panoptes authentication?
I understand and agree with your preference not to use devcert, but I’m not aware of a different way to run the apps locally with auth, which I think is a requirement for certain dev efforts, though please let me know if I'm missing or misunderstanding something.

I'll create an issue to remove devcert so we can document, further discuss, and prioritize accordingly.

@eatyourgreens
Copy link
Contributor

At the moment I use the certificate for local.zooniverse.org that it generated for the project app, but I've removed the CA from my MacBook.

@eatyourgreens
Copy link
Contributor

Thinking about it, a better solution would be for us to generate a real certificate for local.zooniverse.org and use that.

@eatyourgreens
Copy link
Contributor

I'm also assuming that the certificate for the project app can be re-used with the content pages app, since they run on the same domain locally.

@mcbouslog
Copy link
Contributor Author

I've opened #4103 to help document and discuss a way forward towards not using devcert. Thank you @eatyourgreens for highlighting the issues with devcert and the importance of removing the related CA cert.

I'll likely merge this PR later today to continue progress on notifications in app-content-pages (update #4010 ).

@mcbouslog mcbouslog merged commit 873aded into master Jan 9, 2023
@mcbouslog mcbouslog deleted the app-content-pages-dev-server branch January 9, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging dx Things that improve the developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants