Skip to content

Conversation

@gf3
Copy link
Collaborator

@gf3 gf3 commented Jan 24, 2023

Changes

Attempt to both:

  1. Use the provided scheme from the conn if available
  2. Include all ports except 80

Purpose

Two reasons:

  1. We use SSL in development
  2. We require subdomain routing in development (e.g. <id>.local.dev:4000)

@nelsonic nelsonic mentioned this pull request Jan 24, 2023
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Hi @gf3, 👋
Thanks for opening this feedback / enhancement PR. 👌

The build fails because the version of Elixir cannot be loaded:
https://github.com/dwyl/elixir-auth-google/actions/runs/3993114722/jobs/6850008087#step:3:737
image

I've asked @SimonLab to help with #73 and we will update the version of OTP in ci.yml so the build should pass again. Hang tight. ⏳

@SimonLab
Copy link
Member

I've created this PR #76 to fix the CI and tests. Once merged you should be able to rebase your PR.

@nelsonic
Copy link
Member

@SimonLab Thanks for taking a look at this and making the updates. 👌
@gf3 please rebase on main and your PR should pass. 🙏

@gf3
Copy link
Collaborator Author

gf3 commented Jan 24, 2023

Hi @gf3, 👋

Thanks for opening this feedback / enhancement PR. 👌

The build fails because the version of Elixir cannot be loaded:

https://github.com/dwyl/elixir-auth-google/actions/runs/3993114722/jobs/6850008087#step:3:737

image

I've asked @SimonLab to help with #73 and we will update the version of OTP in ci.yml so the build should pass again. Hang tight. ⏳

Thank you 🙏

Also, this is a wonderfully simply package—it is much appreciated.

Attempt to both:

1. Use the provided scheme from the conn if available
2. Include all ports except 80
@gf3
Copy link
Collaborator Author

gf3 commented Jan 24, 2023

@nelsonic @SimonLab rebased!

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #75 (25c1363) into main (3cf0968) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #75   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           28        32    +4     
=========================================
+ Hits            28        32    +4     
Impacted Files Coverage Δ
lib/elixir_auth_google.ex 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nelsonic nelsonic self-assigned this Jan 24, 2023
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@gf3 thanks very much for creating this PR to improve the package. 🎉
It's always great to know people are actually using the code we've written ... 🚀
Your additions are good and tests are succinct. ✅

My only "feedback" is:

  1. Please create an issue to describe the problem you are facing before creating a PR.
  2. Add some sort of docs/usage for the change you made to the README.md so that other people using the package know that they can do the subdomain routing.

Other than that. Great work! 🙌
Welcome to @dwyl you now have write-access to everything! 📝
See:

@nelsonic nelsonic merged commit db1e88b into dwyl:main Jan 25, 2023
nelsonic added a commit that referenced this pull request Jan 25, 2023
@nelsonic
Copy link
Member

Package published to https://hex.pm/packages/elixir_auth_google/1.6.4 📦 contains these updates. 🚀
Thanks again. 🥇

@gf3
Copy link
Collaborator Author

gf3 commented Jan 25, 2023

Thank YOU 🫡

@gf3 gf3 deleted the more-robust-host-config branch January 25, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants