-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Refactor, cleanup Akamai provider #1870
Refactor, cleanup Akamai provider #1870
Conversation
refactor: remove dns api logic and use dns api library enhancement: add additional args for auth credential retieval cleanup: simplify, organize processing logic test: update automation and validate
update README.md to include akamai provider changes update CHANGEME.md to rename akamai-fastdns refs to akamai-edgedns update Akamai tutorial name and content for updated functionality and tested scenarios.
Welcome @edglynes! |
/assign @njuettner |
@njuettner @seanmalloy Hi. I had added prior contributors as reviewers as a courtesy. Don't think they are active. This PR is ready for submission. I've reviewed, ran make lint, make test, make build and make build.docker all successfully and tested. Can one of you review and approve? Else, what do you suggest? Thx. |
/assign @njuettner @Raffo |
@sheerun @tariq1890 - any interest in reviewing this PR given prior contributions? hx. |
I don't use akamai, it needs an owner. Maybe send e-mail to akamai if they are willing to designate someone to support it |
I work for Akamai and am quite willing to be the owner, however I can't review, nor approve, the PR I authored... |
Then you need to find your way into https://github.com/kubernetes/org/blob/master/config/kubernetes-sigs/org.yaml which means you need to find 2 reviewers that are willing to be your sponsors (I know, pain, maybe it's best to ask around at slack channels like #sig-network or #external-dns). Right now I'm only member, not reviewer. Then you'll probably be added to OWNERS file of akamai in this repository which means you would not need as comprehensive reviews from others and you could review and approve PRs for akamai as well |
Here are more details: https://github.com/kubernetes/community/blob/master/community-membership.md#requirements |
All I can do is approve this PR, which I will do because all tests are preserved and changes seems to be refactor-only /approve |
Also note that my approval doesn't mean much because I'm owner of cloudflare provider, not a reviewer of whole repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went trough the code a bit and saw some flaws. Could take another look at your code and review it again you might see some additional things which you want to optimize?
You can ping me again once you feel confident 🙂
- removed commented code - removed unnecessary provider checks in GetRecords and AddChanges - removed noisy debugf comments - updated validation keys check
@njuettner Went thru all feedback and pushed update. Please reviewand approve. Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small nit since we already have released v0.7.4 🤞
Hi @njuettner. I made last changes you requested a few weeks ago. Can you approve this PR and merge? Thx. |
@njuettner happy new year. I rebased today from master. All review comments addressed. Please approve and merge. Thank you. @Raffo @seanmalloy @hjacobs @linki I've not heard back from Nick in over a month. This PR is/has been ready to go. All review comments addressed and recently rebased. Can any of you approve and merge or direct me to someone who can? Thank you. Ed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edglynes, njuettner, sheerun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It seems like this was published in v0.8.0, but now that release seems to have disappeared...? Any idea why? Bit confusing! 😂 |
Ref. #1846
Refactored provider to use Edge DNS API library. Updated library version to 1.0.0. Cleaned up the implementation by organizing into logical functions and removing un necessary logic. Updated test automation to align with logic changes. Added additional optional arguments for API authentication. Updated the tutorial name and content to reflect Akamai DNS product name change and validation.
/kind feature
/cc @Raffo @sheerun @tariq1890