Reference TaskDefinition ARN if resolvable #1028

mhoshi-vm · 2023-12-15T15:14:46Z

Description of your changes

Re-Fix idea for #585

I have:

Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Run this code locally and checked the service to run more than the interval.

% kubectl get service.ecs.aws.upbound.io
NAME           READY   SYNCED   EXTERNAL-NAME   AGE
test-ecs-ns1   True    True     test-ecs-ns1    86m

mhoshi-vm · 2023-12-15T15:17:01Z

Continue of #826 (comment)

mbbush · 2023-12-17T00:38:45Z

/test-examples="examples/ecs/service.yaml"

mbbush · 2023-12-17T01:32:09Z

Thanks for the contribution, @mhoshi-vm! This looks like a better approach than the one you used in the previous PR.

There's a problem with the end-to-end tests. The import step, which is intended to validate that the resource remains stable when e.g. the provider pod dies, is failing because of an issue with the ecs cluster resource:

    logger.go:42: 00:55:05 | case/2-import |     - lastTransitionTime: "2023-12-17T00:53:57Z"
    logger.go:42: 00:55:05 | case/2-import |       message: 'observe failed: failed to set the external-name of the managed resource
    logger.go:42: 00:55:05 | case/2-import |         during observe: failed to get the external-name from ID: example: terraform
    logger.go:42: 00:55:05 | case/2-import |         ID should be the ARN of the cluster'
    logger.go:42: 00:55:05 | case/2-import |       reason: ReconcileError
    logger.go:42: 00:55:05 | case/2-import |       status: "False"
    logger.go:42: 00:55:05 | case/2-import |       type: Synced

My best guess is this is another example of something I've seen often with the terraform aws provider. Sometimes the value of the id that it returns is different depending on whether you created or observed the resource. This is a problem for crossplane.

It looks like both ecs cluster and ecs service have their external name configurations overwritten in ecs/config.go, which is not the norm; most resources have their external name config in externalname.go, even if that requires a custom function.

Problematically, the custom GetExternalNameFn throws an error when the terraform id is not in the expected format, which the import test (which did not exist in May of 2022 when that custom name was committed) shows us happens frequently. So I think we should fix that.

Based on my reading, I'd try just using config.NameAsIdentifier without the custom GetExternalNameFn for the cluster, and using config.TemplatedStringAsIdentifier("name", "{{ .parameters.cluster }}/{{ .external_name }}"), for the service. That should accomplish the same thing as the current custom functions, but without throwing errors.

To get setup to run the e2e tests locally, all you need to do is follow the directions in the makefile to set the UPTEST_CLOUD_CREDENTIALS env var (you can just set credentials for [default] when there's no cross-account access needed), then you can run make e2e UPTEST_EXAMPLE_LIST="examples/ecs/whatever.yaml".

mhoshi-vm · 2023-12-26T15:12:06Z

Not sure why the lint gone into error, but I hope i done this correctly ...

mbbush · 2023-12-30T23:11:10Z

/test-examples="examples/ecs/service.yaml"

config/ecs/config.go

mbbush · 2023-12-31T01:12:25Z

@mhoshi-vm You'll need to remove the manual-intervention annotation on the Service in examples/ecs/service.yaml so we can validate that it's not stuck in an update loop.

mhoshi-vm · 2024-01-05T08:22:36Z

/test-examples="examples/ecs/service.yaml"

mhoshi-vm · 2024-01-05T14:19:06Z

Failed with the following. Trying to resolve

    logger.go:42: 08:49:44 | case/0-apply | - apiVersion: ecs.aws.upbound.io/v1beta1
    logger.go:42: 08:49:44 | case/0-apply |   kind: Cluster
    logger.go:42: 08:49:44 | case/0-apply |   metadata:
    logger.go:42: 08:49:44 | case/0-apply |     annotations:
    logger.go:42: 08:49:44 | case/0-apply |       crossplane.io/external-create-pending: "2024-01-05T08:29:47Z"
    logger.go:42: 08:49:44 | case/0-apply |       crossplane.io/external-create-succeeded: "2024-01-05T08:29:47Z"
    logger.go:42: 08:49:44 | case/0-apply |       crossplane.io/external-name: arn:aws:ecs:us-west-1:153891904029:cluster/example
    logger.go:42: 08:49:44 | case/0-apply |       meta.upbound.io/example-id: ecs/v1beta1/service
    logger.go:42: 08:49:44 | case/0-apply |       upjet.upbound.io/test: "true"
    logger.go:42: 08:49:44 | case/0-apply |     creationTimestamp: "2024-01-05T08:29:46Z"
    logger.go:42: 08:49:44 | case/0-apply |     finalizers:
    logger.go:42: 08:49:44 | case/0-apply |     - finalizer.managedresource.crossplane.io
    logger.go:42: 08:49:44 | case/0-apply |     generation: 3
    logger.go:42: 08:49:44 | case/0-apply |     labels:
    logger.go:42: 08:49:44 | case/0-apply |       testing.upbound.io/example-name: example-cluster
    logger.go:42: 08:49:44 | case/0-apply |     name: example
    logger.go:42: 08:49:44 | case/0-apply |     resourceVersion: "1018"
    logger.go:42: 08:49:44 | case/0-apply |     uid: 2654eb57-c434-4385-94b7-5d7d022ca838
    logger.go:42: 08:49:44 | case/0-apply |   spec:
    logger.go:42: 08:49:44 | case/0-apply |     deletionPolicy: Delete
    logger.go:42: 08:49:44 | case/0-apply |     forProvider:
    logger.go:42: 08:49:44 | case/0-apply |       region: us-west-1
    logger.go:42: 08:49:44 | case/0-apply |       setting:
    logger.go:42: 08:49:44 | case/0-apply |       - name: containerInsights
    logger.go:42: 08:49:44 | case/0-apply |         value: disabled
    logger.go:42: 08:49:44 | case/0-apply |       tags:
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-kind: cluster.ecs.aws.upbound.io
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-name: example
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-providerconfig: default
    logger.go:42: 08:49:44 | case/0-apply |     initProvider: {}
    logger.go:42: 08:49:44 | case/0-apply |     managementPolicies:
    logger.go:42: 08:49:44 | case/0-apply |     - '*'
    logger.go:42: 08:49:44 | case/0-apply |     providerConfigRef:
    logger.go:42: 08:49:44 | case/0-apply |       name: default
    logger.go:42: 08:49:44 | case/0-apply |   status:
    logger.go:42: 08:49:44 | case/0-apply |     atProvider:
    logger.go:42: 08:49:44 | case/0-apply |       arn: arn:aws:ecs:us-west-1:153891904029:cluster/example
    logger.go:42: 08:49:44 | case/0-apply |       id: arn:aws:ecs:us-west-1:153891904029:cluster/example
    logger.go:42: 08:49:44 | case/0-apply |       setting:
    logger.go:42: 08:49:44 | case/0-apply |       - name: containerInsights
    logger.go:42: 08:49:44 | case/0-apply |         value: disabled
    logger.go:42: 08:49:44 | case/0-apply |       tags:
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-kind: cluster.ecs.aws.upbound.io
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-name: example
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-providerconfig: default
    logger.go:42: 08:49:44 | case/0-apply |       tagsAll:
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-kind: cluster.ecs.aws.upbound.io
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-name: example
    logger.go:42: 08:49:44 | case/0-apply |         crossplane-providerconfig: default
    logger.go:42: 08:49:44 | case/0-apply |     conditions:
    logger.go:42: 08:49:44 | case/0-apply |     - lastTransitionTime: "2024-01-05T08:29:58Z"
    logger.go:42: 08:49:44 | case/0-apply |       reason: Available
    logger.go:42: 08:49:44 | case/0-apply |       status: "True"
    logger.go:42: 08:49:44 | case/0-apply |       type: Ready
    logger.go:42: 08:49:44 | case/0-apply |     - lastTransitionTime: "2024-01-05T08:29:47Z"
    logger.go:42: 08:49:44 | case/0-apply |       reason: ReconcileSuccess
    logger.go:42: 08:49:44 | case/0-apply |       status: "True"
    logger.go:42: 08:49:44 | case/0-apply |       type: Synced
    logger.go:42: 08:49:44 | case/0-apply |     - lastTransitionTime: "2024-01-05T08:29:58Z"
    logger.go:42: 08:49:44 | case/0-apply |       message: 'async update failed: refuse to update the external resource: cannot
    logger.go:42: 08:49:44 | case/0-apply |         change the value of the argument "name" from "example" to "arn:aws:ecs:us-west-1:153891904029:cluster/example"'
    logger.go:42: 08:49:44 | case/0-apply |       reason: AsyncUpdateFailure
    logger.go:42: 08:49:44 | case/0-apply |       status: "False"
    logger.go:42: 08:49:44 | case/0-apply |       type: LastAsyncOperation

mhoshi-vm · 2024-01-05T14:20:27Z

/test-examples="examples/ecs/service.yaml"

mhoshi-vm · 2024-01-05T14:34:05Z

@mbbush i think i passed all the tests, please review

mhoshi-vm · 2024-01-05T14:40:19Z

/test-examples="examples/ecs/service-static-cluster.yaml"

mhoshi-vm · 2024-02-14T06:06:39Z

/test-examples="examples/ecs/service.yaml"

github-actions · 2024-06-04T04:28:24Z

This provider repo does not have enough maintainers to address every pull request. Since there has been no activity in the last 90 days it is now marked as stale. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

github-actions · 2024-06-18T04:28:55Z

This pull request is being closed since there has been no activity for 14 days since marking it as stale. If you're still working on this, feel free to reopen the PR or create a new one!

Reference TaskDefinition ARN if resolvable

d32fabb

mhoshi-vm requested review from ulucinar, sergenyalcin and turkenf as code owners December 15, 2023 15:14

mhoshi-vm force-pushed the main branch from ce25cef to d32fabb Compare December 26, 2023 14:52

Reference TaskDefinition ARN if resolvable

dea7cc8

mbbush reviewed Dec 30, 2023

View reviewed changes

config/ecs/config.go Show resolved Hide resolved

mhoshi-vm added 3 commits January 5, 2024 10:29

Fix config.go order and remove service annotation

45242b2

Merge branch 'main' of https://github.com/upbound/provider-aws

7677b03

Pull from main and regenerate

4b5138b

Change template for ecs cluster

a46f7b8

Update example for static cluster also

fdae02e

mhoshi-vm force-pushed the main branch 2 times, most recently from a4ddcfa to fdae02e Compare February 14, 2024 05:36

mhoshi-vm added 2 commits February 14, 2024 14:42

Merge branch 'main' of https://github.com/upbound/provider-aws

119f53e

Update build

c3e969e

turkenf added the breaking-change label Mar 5, 2024

github-actions bot added the stale label Jun 4, 2024

github-actions bot closed this Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference TaskDefinition ARN if resolvable #1028

Reference TaskDefinition ARN if resolvable #1028

mhoshi-vm commented Dec 15, 2023 •

edited

Loading

mhoshi-vm commented Dec 15, 2023

mbbush commented Dec 17, 2023

mbbush commented Dec 17, 2023 •

edited

Loading

mhoshi-vm commented Dec 26, 2023

mbbush commented Dec 30, 2023

mbbush commented Dec 31, 2023

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Feb 14, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 18, 2024

Reference TaskDefinition ARN if resolvable #1028

Reference TaskDefinition ARN if resolvable #1028

Conversation

mhoshi-vm commented Dec 15, 2023 • edited Loading

Description of your changes

How has this code been tested

mhoshi-vm commented Dec 15, 2023

mbbush commented Dec 17, 2023

mbbush commented Dec 17, 2023 • edited Loading

mhoshi-vm commented Dec 26, 2023

mbbush commented Dec 30, 2023

mbbush commented Dec 31, 2023

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Jan 5, 2024

mhoshi-vm commented Feb 14, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 18, 2024

mhoshi-vm commented Dec 15, 2023 •

edited

Loading

mbbush commented Dec 17, 2023 •

edited

Loading