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

capi-add-tags-to-resources #170

Merged
merged 12 commits into from
Nov 6, 2023
Merged

capi-add-tags-to-resources #170

merged 12 commits into from
Nov 6, 2023

Conversation

calvix
Copy link
Contributor

@calvix calvix commented Nov 1, 2023

@calvix calvix self-assigned this Nov 1, 2023
pkg/irsa/eks/eks.go Outdated Show resolved Hide resolved
@calvix calvix requested a review from a team November 2, 2023 12:47
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.

Minor comments and questions, rest LGTM

@@ -127,6 +127,7 @@ func (s *Service) EnsureDistribution(config DistributionConfig) (*Distribution,

// Add internal and customer tags.
{
customerTags := config.CustomerTags
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
customerTags := config.CustomerTags
customerTags := config.CustomerTags.DeepCopy()

so we don't mutate the input parameter which may lead to surprises

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DeepCopy function does not exists for map[], so doing the ugly copy via for

pkg/aws/services/s3/bucket.go Outdated Show resolved Hide resolved
// Fetch custom tags from AWSCluster CR
awsCluster := &capa.AWSCluster{}
err = s.Client.Get(ctx, types.NamespacedName{Namespace: s.Scope.ClusterNamespace(), Name: s.Scope.ClusterName()}, awsCluster)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specifically handle apierrors.IsNotFound(err), as before?

Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea, yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think its a good idea to continue if the AWSCluster CR is missing, because in that case either something terrible happened or it's some kind of super early in the creation processes and it would mean we ignore the tags that are supposed to be there.

this code feels like it was copied from somewhere where it make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it on purpose

@calvix calvix requested a review from a team November 2, 2023 17:12
@calvix calvix enabled auto-merge (squash) November 6, 2023 14:40
@calvix calvix merged commit 7c491ad into master Nov 6, 2023
2 checks passed
@calvix calvix deleted the capi-add-tags-to-resources branch November 6, 2023 14:42
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.

3 participants