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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Ezzmo
Copy link

@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

@Ezzmo
Copy link
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
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
Author

Ezzmo commented Nov 15, 2024

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

@Ezzmo
Copy link
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.

examples/sagemaker/v1beta1/mlflowtrackingserver.yaml Outdated Show resolved Hide resolved
examples/sagemaker/v1beta1/mlflowtrackingserver.yaml Outdated Show resolved Hide resolved
examples/sagemaker/v1beta1/mlflowtrackingserver.yaml Outdated Show resolved Hide resolved
@Ezzmo
Copy link
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
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"),

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