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

Sync DO API keys between clouds #16205

Conversation

amiedes
Copy link
Contributor

@amiedes amiedes commented Mar 8, 2021

@shortcut-integration
Copy link

@amiedes amiedes changed the title Feature/ch132964/sync api keys both in on prem and associated fix specs 3 Sync DO API keys between clouds Mar 9, 2021
@amiedes amiedes marked this pull request as ready for review March 9, 2021 11:09
def create_remote_do_api_key
cartodb_central_topic.publish(
:create_remote_do_api_key,
{ type: type, token: token, user_id: user_id, username: user.username }
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to myself: sending both user_id and username, important to relay remote creation messages

@amiedes amiedes requested a review from rafatower March 9, 2021 14:05
Copy link
Contributor

@rafatower rafatower left a comment

Choose a reason for hiding this comment

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

Sorry, but we have to address a (potential) message ordering issue. After that I think it can be merged.

The rest looks fine to me. Of course you got a special waiver to ignore rubocop complaints in test code, as you had to modify a ton of specs.

@@ -65,15 +68,19 @@ class ApiKey < ActiveRecord::Base
validate :valid_default_public_key, if: :default_public?

after_create :setup_db_role, if: ->(k) { k.needs_setup? && !k.skip_role_setup }
after_create :create_remote_do_api_key, if: ->(api_key) { api_key.master? }
Copy link
Contributor

Choose a reason for hiding this comment

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

just an opinion: I like more the syntax we use in Central

after_create(:create_remote_do_api_key, if: :master?)

(I understand you do need a block or a new function predicate for the check below (regenerate callback))
(as an opinion and likes and dislikes you're entitled to completely ignore it)

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 for the suggestion 👍 . I agree with you and will try to remember this for the next time, but for (lazyness? 😅 ) I'm going to deploy it like it's now to avoid having to wait for the build again and potentially having to do a retry

)
end

def regenerate_remote_do_api_key
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but we need a specific message here: there's no guarantee on the order of the messages, and the likelihood of the "create" to overtake the "destroy" is pretty high in this case.

I think it is safer to have a specific message/command :regenerate_remote_do_api_key almost for the same price.

Copy link
Contributor

Choose a reason for hiding this comment

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

After our chat and for the record: it is OK as worst case scenario is having an old api key in redis for a short while, as old and new have different redis keys.

OTOH that triggered the need for this: https://github.com/CartoDB/data-observatory/pull/1010

Copy link
Contributor

@rafatower rafatower left a comment

Choose a reason for hiding this comment

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

👍

@amiedes amiedes merged commit b1c998d into master Mar 10, 2021
@amiedes amiedes deleted the feature/ch132964/sync-api-keys-both-in-on-prem-and-associated-fix-specs-3 branch March 10, 2021 08:05
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.

2 participants