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

Add aws_sagemaker_mlflow_tracking_server resource #1564

Closed
wants to merge 27 commits into from

Conversation

Ezzmo
Copy link
Contributor

@Ezzmo Ezzmo commented Nov 13, 2024

Description of your changes

This PR adds the mlflowtrackingservers.sagemaker.aws.upbound.io resource

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

I tested locally with uptest. The MlFlow tracking server created successfully and tasks succeeded

@Ezzmo Ezzmo force-pushed the mlflow_tracking_server branch from e7ece3b to 08496b3 Compare November 13, 2024 15:45
@Ezzmo
Copy link
Contributor Author

Ezzmo commented Nov 13, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

@Ezzmo Ezzmo marked this pull request as ready for review November 13, 2024 16:06
@Ezzmo
Copy link
Contributor Author

Ezzmo commented Nov 13, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

2 similar comments
@turkenf
Copy link
Collaborator

turkenf commented Nov 13, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

@Ezzmo
Copy link
Contributor Author

Ezzmo commented Nov 15, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

@Ezzmo
Copy link
Contributor Author

Ezzmo commented Nov 15, 2024

@turkenf do you mind running the tests again? The error may have been transient, doesn't look code related

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

@jeanduplessis
Copy link
Collaborator

@Ezzmo It looks like the provider pod logs have some info on errors

provider-aws-sagemaker logs
provider-aws-iam logs

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 contribution @Ezzmo. When we look at the uptest logs, we see that the resources cannot resolve the references. I left comments about this for you to consider.

@Ezzmo Ezzmo force-pushed the mlflow_tracking_server branch 2 times, most recently from 5ff8a59 to 835d924 Compare November 26, 2024 11:59
@Ezzmo
Copy link
Contributor Author

Ezzmo commented Nov 26, 2024

Thanks @turkenf , I missed those (oops)

I've updated the references for the related role and policy, fixed the S3 reference and some metadata mistakes

@turkenf
Copy link
Collaborator

turkenf commented Nov 26, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

@Ezzmo
Copy link
Contributor Author

Ezzmo commented Nov 27, 2024

@turkenf Looks like it needs another run

@@ -2281,6 +2281,8 @@ var TerraformPluginSDKExternalNameConfigs = map[string]config.ExternalName{
"aws_sagemaker_image": config.ParameterAsIdentifier("image_name"),
// SageMaker Code Images can be imported using the name
"aws_sagemaker_image_version": config.IdentifierFromProvider,
// Sagemaker MLFlow tracking server can be imported using the name
"aws_sagemaker_mlflow_tracking_server": config.NameAsIdentifier,
Copy link
Collaborator

Choose a reason for hiding this comment

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

To use NameAsIdentifier as an external name, the resource must be imported with the name, and the name argument must be present. See https://github.com/crossplane/upjet/blob/main/docs/adding-new-resource.md#case-1-name-as-identifier

When I look at the resource's import document, it says that it can be imported with workteam_name. I assume there is a mistake here and that is because I don't see an argument called workteam_name. Instead, tracking_server_name can be used, and in this case, the most appropriate external name configuration is ParameterAsIdentifier.

Could you please regenerate the resource with the following configuration?

Suggested change
"aws_sagemaker_mlflow_tracking_server": config.NameAsIdentifier,
"aws_sagemaker_mlflow_tracking_server": config.ParameterAsIdentifier("tracking_server_name"),

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 made the mistake of thinking id was acceptable for NameAsIdentifier

Thanks @turkenf. I've updated

@Ezzmo Ezzmo force-pushed the mlflow_tracking_server branch from a939d29 to f957e6c Compare December 4, 2024 15:09
@turkenf
Copy link
Collaborator

turkenf commented Dec 10, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

1 similar comment
@turkenf
Copy link
Collaborator

turkenf commented Dec 10, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

Ezzmo and others added 7 commits December 11, 2024 15:35
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
…tionDetails

Signed-off-by: Tim Birkett <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Tim Birkett <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Tim Birkett <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Ezzmo and others added 15 commits December 11, 2024 15:39
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Cem Mergenci <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Cem Mergenci <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Ezzmo <[email protected]>
@Ezzmo Ezzmo force-pushed the mlflow_tracking_server branch from 927082c to 624bc2a Compare December 11, 2024 15:39
@Ezzmo
Copy link
Contributor Author

Ezzmo commented Dec 11, 2024

@turkenf Thanks for helping in the Slack channel. Tests ran successfully finally after increasing the timeout.

--- PASS: kuttl (2843.65s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/case (2842.41s)

Hopefully will run successfully here

@turkenf
Copy link
Collaborator

turkenf commented Dec 11, 2024

/test-examples="examples/sagemaker/v1beta1/mlflowtrackingserver.yaml"

https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/12280429287

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.

Many thanks for all your efforts @Ezzmo, the PR looks good to me, except for the commit history :) If you have time, it would be great to squash your commits to keep the commit history clean.

@Ezzmo
Copy link
Contributor Author

Ezzmo commented Dec 13, 2024

@turkenf Rebasing this is proving very hairy, I'm going to produce a new branch with a new PR with a single commit to clean this all up. I suspect the way I updated my feature branch has caused problems now.

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.

5 participants