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.

Sign up for GitHub

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

Jump to bottom

refactoring #191

Merged
merged 10 commits into from
Oct 3, 2023
Merged

refactoring #191

merged 10 commits into from
Oct 3, 2023

Conversation

bdehri
Copy link

@bdehri bdehri commented Sep 15, 2023 •
edited
Loading

  • add tests to iam package
  • refactor reconcilers

@bdehri bdehri self-assigned this Sep 15, 2023
@bdehri bdehri force-pushed the refactor branch 5 times, most recently from 8fec3d5 to 3622b31 Compare September 15, 2023 14:46
@bdehri bdehri force-pushed the refactor branch 4 times, most recently from d44bc8a to 267d759 Compare September 18, 2023 15:12
@bdehri bdehri marked this pull request as ready for review September 18, 2023 15:19
@bdehri bdehri requested a review from a team as a code owner September 18, 2023 15:19
AndiDog
AndiDog reviewed Sep 19, 2023
View reviewed changes
Copy link
Contributor

@AndiDog AndiDog left a comment

Choose a reason for hiding this comment

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

Not a full review yet

pkg/key/key.go Show resolved Hide resolved
@bdehri
Copy link
Author

bdehri commented Sep 27, 2023

I reverted the latest change as it introduced deadlock between awsMachinePool and awsCluster.

@AndiDog
Copy link
Contributor

AndiDog commented Sep 28, 2023 •
edited
Loading

I reverted the latest change as it introduced deadlock between awsMachinePool and awsCluster.

Unfortunately, I don't have a great idea to solve this. We need the machine pool name for deletion, so we can't remove the AWSMachinePool finalizer first either. Let's go ahead as-is and maybe we'll find a solution later. This is not a large problem, but leads to false error logs during deletion (AWSCluster reconciler failing to reconcile S3 bucket IAM policy once the IAM roles are missing).

Tracking issue: giantswarm/roadmap#2715

I'll review this one more time.

@bdehri bdehri merged commit e57c7ef into master Oct 3, 2023
2 checks passed
@bdehri bdehri deleted the refactor branch October 3, 2023 07:24
@mnitchev
Copy link
Member

Towards giantswarm/roadmap#2113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fiunchinho fiunchinho fiunchinho left review comments

@AndiDog AndiDog AndiDog approved these changes

Assignees

@bdehri bdehri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bdehri @AndiDog @mnitchev @fiunchinho