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

feat: aws_ec2_fleet resource #1405

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Conversation

teabot
Copy link
Contributor

@teabot teabot commented Jul 18, 2024

Description of your changes

Added aws_ec2_fleet resource to the provider.

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Local run of uptest:

> export UPTEST_EXAMPLE_LIST=examples/ec2/v1beta1/fleetrequest.yaml
> make e2e
...
=== CONT  kuttl
    harness.go:402: run tests finished
    harness.go:511: cleaning up
    harness.go:553: skipping cluster tear down
    harness.go:554: to connect to the cluster, run: export KUBECONFIG="/Users/elliot.west/git/provider-upjet-aws/kubeconfig"
--- PASS: kuttl (50.59s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/case (49.81s)
PASS
19:26:45 [ OK ] running automated tests

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/ec2/v1beta1/fleetrequest.yaml"

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you for your effort @teabot, I left a few comments for you to consider.

examples/ec2/v1beta1/fleetrequest.yaml Outdated Show resolved Hide resolved
examples/ec2/v1beta1/fleetrequest.yaml Outdated Show resolved Hide resolved
examples/ec2/v1beta1/fleetrequest.yaml Outdated Show resolved Hide resolved
examples/ec2/v1beta1/fleetrequest.yaml Outdated Show resolved Hide resolved
@teabot
Copy link
Contributor Author

teabot commented Jul 25, 2024

Thanks for the feedback @turkenf. I have applied the changes requested. Note that in this providers, the existing spot fleet nomeclature includes a Request suffix. That may also be considered to be erroneous, but I'll consider it outside of the scope of this ticket.

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/ec2/v1beta1/fleetrequest.yaml"

@teabot
Copy link
Contributor Author

teabot commented Jul 25, 2024

/test-examples="examples/ec2/v1beta1/fleet.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Jul 25, 2024

/test-examples="examples/ec2/v1beta1/fleet.yaml"

@teabot
Copy link
Contributor Author

teabot commented Jul 25, 2024

This example passes local upsets, however I notice the following issues, and would appreciate help solving them:

  1. forProvider.launchTemplateConfig.launchTemplateSpecification.versionSelector does not function properly and returns null despite all launch templates having a version (ec2: Fleet missing reference to launch template version #118). Please advise where I might look to fix this. I bypass this by setting a value of (string)"1", when we might also reasonably expect this to be an integer. Note that this does't affect my use-case, and so I'd been keen for this PR to move forward regardless, however I would prefer to fix this.
  2. In TF, --no-terminate-instances seems to be set by default to true for fleet type instant, which is not allowed. This is perhaps a TF issue, but if there is a way to manage this behaviour in this provider then please let me know.
  3. Some properties, that we might know to accept only integers or booleans, require a string type. Presumably this is erroneously defined/resolved from TF and enforced in the CRD. If there is a way to correct this behaviour locally in this provider, please let me know.

cc. @turkenf

@teabot teabot marked this pull request as ready for review July 25, 2024 18:55
@teabot teabot changed the title ec2_fleet feat: aws_ec2_fleet resource Jul 25, 2024
@turkenf
Copy link
Collaborator

turkenf commented Jul 27, 2024

/test-examples="examples/ec2/v1beta1/fleet.yaml"

@teabot
Copy link
Contributor Author

teabot commented Aug 1, 2024

@turkenf Is anything else required here?

@teabot teabot requested a review from turkenf August 1, 2024 12:13
@turkenf
Copy link
Collaborator

turkenf commented Aug 1, 2024

Thank you very much for your interest @teabot.

About the issues and solutions you noticed in this comment, let's open the issues and discuss them there. There is already an issue with missing reference, you can open issues for other ones. I think these issues are not a blocker for us to move this PR forward.

@turkenf
Copy link
Collaborator

turkenf commented Aug 1, 2024

@teabot
Copy link
Contributor Author

teabot commented Aug 6, 2024

@teabot teabot force-pushed the resource/fleet branch 2 times, most recently from fe8ee30 to a57a3d4 Compare August 7, 2024 08:28
@teabot teabot requested a review from turkenf August 7, 2024 08:46
@turkenf
Copy link
Collaborator

turkenf commented Aug 7, 2024

/test-examples="examples/ec2/v1beta1/fleet.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/10283336838

teabot added 3 commits August 7, 2024 14:43
Signed-off-by: Elliot West <elliot.west@dremio.com>

Update examples/ec2/v1beta1/fleetrequest.yaml

Co-authored-by: Fatih Türken <103541666+turkenf@users.noreply.github.com>
Signed-off-by: Elliot West <elliot.west@dremio.com>

Update examples/ec2/v1beta1/fleetrequest.yaml

Co-authored-by: Fatih Türken <103541666+turkenf@users.noreply.github.com>
Signed-off-by: Elliot West <elliot.west@dremio.com>

Update examples/ec2/v1beta1/fleetrequest.yaml

Co-authored-by: Fatih Türken <103541666+turkenf@users.noreply.github.com>
Signed-off-by: Elliot West <elliot.west@dremio.com>

Passing fleet

Signed-off-by: Elliot West <elliot.west@dremio.com>
Signed-off-by: Elliot West <elliot.west@dremio.com>
Signed-off-by: Elliot West <elliot.west@dremio.com>
Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@turkenf turkenf merged commit 34f77c2 into crossplane-contrib:main Aug 7, 2024
11 checks passed
@teabot teabot deleted the resource/fleet branch August 7, 2024 16:16
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