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

238: Implement keys endpoints to list access keys #239

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

dodie
Copy link
Contributor

@dodie dodie commented Sep 21, 2020

Hi @cdancy ,

This is a draft PR to implement the keys API endpoint discussed in #238 .
I've tested the new API manually, but I'm not finished with the automated tests yet.

I'm opening this draft PR, hoping to ask for feedback if I'm going to the right direction with the implementation:

  • Is it okay to use the already existing AccessKey model objects for this endpoint? Semantically the new usecase uses them similarly as they are used in the already existing endpoint, but here we need a few additional optional parameters.
  • Is it okay to not cover only a part of the Keys API? For my usecase, I only need querying and deleting access keys, but not creating and updating them.

Thanks,
David

@cdancy cdancy self-requested a review September 22, 2020 13:21
@cdancy
Copy link
Owner

cdancy commented Sep 22, 2020

@dodie yeah it actually looks great so far! Can't spot anything off. Would just need mock and integTests for each of these new endpoints and we will be good to go.

@dodie dodie force-pushed the feature/keys-api branch 2 times, most recently from 3cac4f0 to e111f99 Compare September 28, 2020 07:40
@dodie dodie marked this pull request as ready for review September 28, 2020 09:05
@dodie
Copy link
Contributor Author

dodie commented Sep 28, 2020

Hi @cdancy ,

In the end I also created the endpoint methods for creating keys because they were needed in the integration tests.
I also added the mock and integration tests.

Please let me know if anything needs to be further aligned.

Also, I noticed that the build is now failing for an unrelated error, it seems that the InsightsApiLiveTest class has wrong formatting. Shall I fix that as well in this PR?

Cheers,
David

@cdancy
Copy link
Owner

cdancy commented Sep 28, 2020

Also, I noticed that the build is now failing for an unrelated error, it seems that the InsightsApiLiveTest class has wrong formatting. Shall I fix that as well in this PR?

@dodie yes if you don't mind :) I've got no complaints on the test side of things. When you're ready to move forward and merge just ping me here and I'll kick a new release. This looks really great and thanks!

@cdancy cdancy added this to the v2.7.0 milestone Sep 28, 2020
@cdancy cdancy linked an issue Sep 28, 2020 that may be closed by this pull request
@dodie
Copy link
Contributor Author

dodie commented Sep 28, 2020

@cdancy : I've added the missing formatting fixes, now the project builds successfully.
From my point of view, the KeysApi is ready to be released! 🚀

@cdancy cdancy merged commit e68a216 into cdancy:master Sep 29, 2020
@cdancy
Copy link
Owner

cdancy commented Sep 29, 2020

@dodie 2.7.0 has been released and thanks again for the PR!

@dodie
Copy link
Contributor Author

dodie commented Sep 29, 2020

@cdancy 👍
Thank you for the quick release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement keys endpoints to list access keys
2 participants