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

fix(_rotate_kms_key): remove SkipPerIssues since the ticket is resolved #9153

Merged

Conversation

mikliapko
Copy link
Contributor

@mikliapko mikliapko commented Nov 6, 2024

As per discussion in #9134, this clause can be removed as the ticket is resolved and doesn't have sct skip label (what could have made it being skipped). At the same, it led to unexpected failures when self.params appeared to be dict type instance instead of SCTConfiguration as expected.

Testing

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

As per discussion, this clause can be removed as the ticket is resolved
and doesn't have sct skip label (what could have made it being skipped).
At the same, it led to unexpected failures when self.params appeared to
be dict type instance instead of SCTConfiguration as expected.

refs:
- scylladb#9134
@mikliapko mikliapko self-assigned this Nov 6, 2024
@mikliapko mikliapko added backport/2023.1 Need to backport to 2023.1 backport/2024.1 Need backport to 2024.1 backport/2024.2 Need backport to 2024.2 labels Nov 6, 2024
@fruch
Copy link
Contributor

fruch commented Nov 6, 2024

@mikliapko

yes we can remove this code, but I would suggest going to the root cause of why params argument wasn't set creating the cluster, we shouldn't let it happen.

@fruch
Copy link
Contributor

fruch commented Nov 6, 2024

I think the actual fix was suppose to be:

s/self.params/db_cluster.params/

@mikliapko
Copy link
Contributor Author

@mikliapko

yes we can remove this code, but I would suggest going to the root cause of why params argument wasn't set creating the cluster, we shouldn't let it happen.

Yes, totally agree with you and I raised a bug about it (#9134) that remains open.

@mikliapko
Copy link
Contributor Author

I think the actual fix was suppose to be:

s/self.params/db_cluster.params/

okay, but still it looks like this peace of code I'm removing is not relevant anymore since the original issue is closed and not labeled as sct-...-skip.

@mikliapko mikliapko marked this pull request as ready for review November 7, 2024 11:31
@mikliapko mikliapko requested review from a team, vponomaryov and fruch November 7, 2024 11:31
Copy link
Contributor

@vponomaryov vponomaryov left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/2023.1 Need to backport to 2023.1 backport/2024.1 Need backport to 2024.1 backport/2024.2-done Commit backported to 2024.2 promoted-to-master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants