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

[GT-95] Prepare for API credential management based on renewal time #447

Merged
merged 7 commits into from May 4, 2023
Merged

[GT-95] Prepare for API credential management based on renewal time #447

merged 7 commits into from May 4, 2023

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2023

Refactor existing last-use-time code and add prototype test as for #438

@ghost ghost self-requested a review as a code owner March 29, 2023 11:33
@gregcorbett
Copy link
Member

I've tweaked this slightly to reflect that I want unrenewed API credentials to be deleted at a quicker cadence than inactive users. This is to prevent inactive users having API credentials.

@gregcorbett
Copy link
Member

I was slightly surprised the tests passed - as I naively assumed "Well, the deletion of unrenewed credentials hasn't been implemented yet, so the test should fail".

But the logic is almost the same (and hence the logic is reused) as deleting unused credentials, so it makes sense that the tests pass 😄

@gregcorbett
Copy link
Member

rebasing on latest dev

@gregcorbett
Copy link
Member

Why is the PR picking up the commits from #209 following a rebase?

@gregcorbett gregcorbett changed the title Prepare for API credential management based on renewal time [GT-95] Prepare for API credential management based on renewal time Apr 11, 2023
@gregcorbett
Copy link
Member

Why is the PR picking up the commits from #209 following a rebase?

So Git (not GitHub) thinks the merge-base (best common ancestor) is fab7bd0, which is not the latest pull request merged into dev - hence picking up the commits from #209.

$ git merge-base api-renewal-test dev
fab7bd03aaa795062bacd254c495fe7f9115a7db

Ian Neilson and others added 6 commits May 4, 2023 10:16
Basic renaming to make way for
managing renewal cycle tests
Addressing review comment and correct some
code comments and function phpdoc
Co-authored-by: Rowan <rowanmoss04@gmail.com>
- this is to reflect that we want unrenewed API credentials to be
  deleted at a quicker cadence than users.
- this is to prevent inactive users having API credentials.
@gregcorbett
Copy link
Member

gregcorbett commented May 4, 2023

So Git (not GitHub) thinks the merge-base (best common ancestor) is fab7bd0, which is not the latest pull request merged into dev - hence picking up the commits from #209.

Following this StackOverflow answer to try and resolve this: https://stackoverflow.com/a/70994400

$ git rebase --onto dev fab7bd03aaa795062bacd254c495fe7f9115a7db api-renewal-test
Successfully rebased and updated refs/heads/api-renewal-test.

$ git merge-base api-renewal-test dev
2790f307a10457acce4e4567916f5bdf31534544

2790f30 is the latest PR merged into dev, so yay!

A subsequent force push seems to have resolved the weirdness.

Copy link
Member

@rowan04 rowan04 left a comment

Choose a reason for hiding this comment

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

lgtm :)

Co-authored-by: Rowan <rowanmoss04@gmail.com>
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.

2 participants