aws-sdk-go-v2 - v1.3.0 #602

smcavallo · 2021-03-21T18:18:48Z

Description of your changes

aws-sdk-go-v2 latest - https://github.com/aws/aws-sdk-go-v2/releases/tag/v1.3.0

go 1.16 supported for providers as part of upbound/build#140

Fixes #

#383

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Unit tests only

muvaf

Thank you both @enderv and @smcavallo for driving this huge PR!

apis/ec2/v1beta1/referencers.go

pkg/clients/acm/certificate.go

pkg/clients/aws.go

muvaf · 2021-04-07T21:54:42Z

pkg/clients/aws.go

-	}
-	config, err := external.LoadDefaultAWSConfig(shared)
-	return &config, err
+	return &cfg, err


I will defer to @hasheddan for reviewing these authentication parts.

muvaf · 2021-04-07T21:55:46Z

pkg/clients/aws.go

-	if awsErr, ok := err.(awserr.Error); ok {
-		return awserr.New(awsErr.Code(), awsErr.Message(), nil)
-	}
+	// TODO cvodak revisit this


FWIW, we cannot merge without handling this.

Would you be able to test whether we need this for this v2 SDK? The reason we have it is to remove the request id from the error so that we don't get re-queued immediately since there is a change at every reconcile. If v2 errors don't contain a unique request id, then it might be fine not to have any handling here.

muvaf · 2021-04-07T21:57:18Z

pkg/clients/cloudformation/cloudformation.go

@@ -18,49 +18,49 @@ package cloudformation



I think this pkg/clients/cloudformation folder is remnant of old EKS cluster creation. We can delete the whole folder as it's not used anywhere.

pkg/clients/dbsubnetgroup/dbsubnetgroup.go

chlunde · 2021-04-19T20:47:25Z

Wow, this is a huge PR, I had not seen it. I wonder if provider-aws should have a freeze until this is merged? I suspect it would be hard to prevent loosing commits/bug fixes or get merge conflict related bugs. if we don't get this in soon.

I'm sure you already have thought about a plan here, but I think it might be a good idea to communicate it with a pinned issue. Will there be a release first, and then focus on this before allowing further changes?

muvaf · 2021-04-22T23:34:42Z

@chlunde I don't think we have a written plan but having a release -> merging this -> having another release right after that could be a good idea.

I wonder if provider-aws should have a freeze until this is merged?

Most changes coming into provider-aws are code-generated resources which use v1 SDK, so I don't think we need a freeze for PRs.

apis/ec2/v1beta1/referencers.go

muvaf · 2021-04-22T23:55:19Z

pkg/clients/aws.go

-	if awsErr, ok := err.(awserr.Error); ok {
-		return awserr.New(awsErr.Code(), awsErr.Message(), nil)
-	}
+	// TODO cvodak revisit this


Would you be able to test whether we need this for this v2 SDK? The reason we have it is to remove the request id from the error so that we don't get re-queued immediately since there is a change at every reconcile. If v2 errors don't contain a unique request id, then it might be fine not to have any handling here.

janwillies · 2021-06-21T17:43:18Z

@chlunde I don't think we have a written plan but having a release -> merging this -> having another release right after that could be a good idea.

seeing that 0.19 was released a few days ago, maybe now is a good time? It would unblock a few things for us (e.g. #610, #338 )

annabarnes1138 · 2021-08-19T12:51:12Z

What is the plan for merging this?

Signed-off-by: smcavallo <[email protected]>

muvaf · 2021-09-28T18:35:26Z

Thanks for pushing an update @smcavallo ! The PR was waiting an action from me but I couldn't get to it for quite a while, having a month-long leave in between.

I'll review as soon as possible.

muvaf

@smcavallo seems like there are compilation errors, some of them because of the diff lines added by git during resolution conflict. Could you make sure no error is thrown when you run make reviewable?

muvaf · 2021-09-29T19:27:10Z

package/crds/acmpca.aws.crossplane.io_certificateauthoritypermissions.yaml

@@ -1,3 +1,5 @@
+


I think we need to update build submodule to latest to so that crd cleanup runs properly to delete these two lines.

muvaf · 2021-09-29T19:28:37Z

pkg/clients/rds/rds.go

 			}
 		}
 	}
 	if db.PendingModifiedValues != nil {
 		o.PendingModifiedValues = v1beta1.PendingModifiedValues{
+<<<<<<< HEAD


I think this is a remnant from merge conflict resolution that we need to remove.

muvaf · 2021-09-29T19:30:56Z

pkg/clients/acm/certificate.go

@@ -73,6 +75,9 @@ func GenerateCreateCertificateInput(name string, p *v1alpha1.CertificateParamete
 }

 // GenerateCertificateStatus is used to produce CertificateExternalStatus from acm.certificateStatus
+<<<<<<< HEAD


I think this is remnant from the conflict resolution we need to remove.

haarchri · 2021-10-02T17:31:40Z

we need this also to support Spot Instances for EKS #228

muvaf

I'll merge this PR and open another small one to fix the issues so that we get the rebasing stuff out of the way.

@enderv @smcavallo Thank you so much for this PR! It's a great contribution that will unblock many use cases users have been waiting for 🙂

smcavallo changed the title ~~WIP: aws-sdk-go-v2 - v1.3.0~~ aws-sdk-go-v2 - v1.3.0 Mar 22, 2021

muvaf self-assigned this Mar 25, 2021

muvaf removed their assignment Mar 25, 2021

muvaf reviewed Apr 7, 2021

View reviewed changes

muvaf reviewed Apr 22, 2021

View reviewed changes

annabarnes1138 mentioned this pull request Jun 4, 2021

Support for Launch Templates #338

Closed

tohjustin mentioned this pull request Aug 11, 2021

ElastiCache: Support LogDeliveryConfigurations field in CacheCluster & ReplicationGroup resources #799

Closed

AaronME assigned muvaf Sep 24, 2021

upgrade to go 1.16, upgrade aws-sdk-go-v2 libraries and fix unit tests

fede498

Signed-off-by: smcavallo <[email protected]>

smcavallo force-pushed the aws-sdk-go-v2_v1.3.0 branch from 3f1bdd7 to fede498 Compare September 28, 2021 13:44

muvaf reviewed Sep 29, 2021

View reviewed changes

muvaf approved these changes Oct 4, 2021

View reviewed changes

muvaf merged commit f282891 into crossplane-contrib:master Oct 4, 2021

This was referenced Oct 4, 2021

Complete AWS SDK v2 Migration #855

Merged

Update AWS SDK v2 version #383

Closed

negz mentioned this pull request Oct 28, 2021

cannot create Kubernetes client: Unauthorized #892

Closed

haarchri mentioned this pull request Oct 31, 2021

fix(eks-kubeconfig): eks-presignGetCallerIdentity #901

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws-sdk-go-v2 - v1.3.0 #602

aws-sdk-go-v2 - v1.3.0 #602

smcavallo commented Mar 21, 2021 •

edited

Loading

muvaf left a comment

muvaf Apr 7, 2021

muvaf Apr 7, 2021

muvaf Apr 22, 2021

muvaf Apr 7, 2021

chlunde commented Apr 19, 2021

muvaf commented Apr 22, 2021

muvaf Apr 22, 2021

janwillies commented Jun 21, 2021

annabarnes1138 commented Aug 19, 2021

muvaf commented Sep 28, 2021

muvaf left a comment

muvaf Sep 29, 2021

muvaf Sep 29, 2021

muvaf Sep 29, 2021

haarchri commented Oct 2, 2021

muvaf left a comment

aws-sdk-go-v2 - v1.3.0 #602

aws-sdk-go-v2 - v1.3.0 #602

Conversation

smcavallo commented Mar 21, 2021 • edited Loading

Description of your changes

How has this code been tested

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlunde commented Apr 19, 2021

muvaf commented Apr 22, 2021

Choose a reason for hiding this comment

janwillies commented Jun 21, 2021

annabarnes1138 commented Aug 19, 2021

muvaf commented Sep 28, 2021

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haarchri commented Oct 2, 2021

muvaf left a comment

Choose a reason for hiding this comment

smcavallo commented Mar 21, 2021 •

edited

Loading