Skip to content

Add AAAA Support #136

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

Merged
merged 8 commits into from
Sep 10, 2021
Merged

Add AAAA Support #136

merged 8 commits into from
Sep 10, 2021

Conversation

jriggins
Copy link
Contributor

@jriggins jriggins commented Sep 3, 2021

No description provided.

@jriggins jriggins self-assigned this Sep 3, 2021
@jriggins jriggins force-pushed the aaaa-support/jriggins branch 3 times, most recently from c871cb3 to ced3fba Compare September 3, 2021 23:05
Copy link
Contributor

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Hi! I know this is still a work in progress but wanted to drop a comment about the 192 addresses. :)

@jriggins jriggins force-pushed the aaaa-support/jriggins branch from e4e1168 to b8d52f0 Compare September 7, 2021 21:45
@jriggins jriggins marked this pull request as ready for review September 7, 2021 21:54
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

👍🏾 on most of this but will defer an approval after a second read as this is a good chunk to review.

Specifically, the tests look good and the updates to update-cdn-ips are 👌🏾 too. I'm still weighing the pros and cons around merging A and AAAA checks into single functions as it makes for an easier implementation but increases risk around misuse of the API.

@jriggins
Copy link
Contributor Author

jriggins commented Sep 8, 2021

ℹ️ Going to make some updates.

@jriggins jriggins requested a review from jcudit September 8, 2021 20:54
@jriggins jriggins force-pushed the aaaa-support/jriggins branch from f9146ea to fe48335 Compare September 8, 2021 23:46
@jriggins
Copy link
Contributor Author

jriggins commented Sep 8, 2021

Ready for another review. 🚀

@jriggins jriggins requested a review from a team September 9, 2021 14:35
`git add --verbose #{file}`
end

def parse_cdn_response(source, ips)
send("parse_#{source}", ips)
Copy link
Contributor

Choose a reason for hiding this comment

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

🍜 not a blocker, but I think it's more readable here to

case source
when "fastly"
  # ...
when "cloudflare"
  # ...
else
  raise "oh no #{source}"
end

TooManyBees
TooManyBees previously approved these changes Sep 9, 2021
Copy link
Contributor

@TooManyBees TooManyBees left a comment

Choose a reason for hiding this comment

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

code looks 👍🏼

jcudit
jcudit previously approved these changes Sep 9, 2021
@jriggins jriggins dismissed stale reviews from jcudit and TooManyBees via 1f2d88f September 10, 2021 14:28
@jriggins jriggins force-pushed the aaaa-support/jriggins branch from d2d981c to 2786ce0 Compare September 10, 2021 14:49
2606:50c0:8000::153
2606:50c0:8001::153
2606:50c0:8002::153
2606:50c0:8003::153
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 , but just for confirmation:

curl https://api.github.com/meta | jq .pages | grep ::

  "2606:50c0:8000::153/128",
  "2606:50c0:8001::153/128",
  "2606:50c0:8002::153/128",
  "2606:50c0:8003::153/128"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Yeah I bet I had a bad block cut on the CIDR and left the 8 🙈

@jriggins jriggins merged commit c5ac328 into master Sep 10, 2021
@jriggins jriggins deleted the aaaa-support/jriggins branch September 10, 2021 21:16
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.

5 participants