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

Bump aws-sdk-go to v1.50.15 #257

Merged
merged 12 commits into from
Feb 24, 2024
Merged

Conversation

suryans-commit
Copy link
Member

@suryans-commit suryans-commit commented Feb 16, 2024

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot requested a review from a-hilaly February 16, 2024 22:47
@ack-prow ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 16, 2024
Copy link

ack-prow bot commented Feb 16, 2024

Hi @suryans-commit. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ryansteakley
Copy link
Member

/ok-to-test

@ack-prow ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 16, 2024
Copy link
Member

@a-hilaly a-hilaly 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 @suryans-commit! I left few comments in line

build_hash: 947081ffebdeefcf2c61c4ca6d7e68810bdf9d08
go_version: go1.22.0
version: v0.30.0
api_directory_checksum: 62a1b01c505efd55545d306c2353b9673ce344fb
version: v0.29.2-8-g947081f
Copy link
Member

Choose a reason for hiding this comment

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

Please regenerate using latest code-generator version/commit

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

// Sets whether all model containers deployed to the endpoint are isolated.
// If they are, no inbound or outbound network calls can be made to or from
// the model containers.
EnableNetworkIsolation *bool `json:"enableNetworkIsolation,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what's your naming convention in this controller, in some others we swap those to something like NetworkIsolationEnabled

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like the convention in sagemaker controller is Enable first:

EnableNetworkIsolation *bool `json:"enableNetworkIsolation,omitempty"`

//
// To be able to pass this role to Amazon SageMaker, the caller of this action
// must have the iam:PassRole permission.
ExecutionRoleARN *string `json:"executionRoleARN,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This field need an IAM Role reference

Comment on lines +578 to +583
ExecutionRole *string `json:"executionRole,omitempty"`
}

// The specifications of an instance group that you need to define.
type ClusterInstanceGroupSpecification struct {
ExecutionRole *string `json:"executionRole,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

There at least 10-ich occurrences of fields that needs references in both types.go and the other resource files

// on How to set the EULA acceptance when fine-tuning a model using the AutoML
// API (https://docs.aws.amazon.com/sagemaker/latest/dg/autopilot-create-experiment-finetune-llms.html#autopilot-llms-finetuning-api-optional-params).
type ModelAccessConfig struct {
AcceptEula *bool `json:"acceptEula,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be

AcceptEula *bool `json:"acceptEULA,omitempty"`

If yes then we need to add that the names package in github.com/aws-controllers-k8s/pkg

@@ -6,4 +6,4 @@ kind: Kustomization
images:
- name: controller
newName: public.ecr.aws/aws-controllers-k8s/sagemaker-controller
newTag: 1.2.7
newTag: 1.2.6
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep the same tag in here. Maybe git fetch --tags --all could help

@suryans-commit
Copy link
Member Author

/retest

@suryans-commit
Copy link
Member Author

/retest

@suryans-commit
Copy link
Member Author

/retest

3 similar comments
@suryans-commit
Copy link
Member Author

/retest

@a-hilaly
Copy link
Member

/retest

@suryansh09
Copy link

/retest

@suryans-commit
Copy link
Member Author

/retest

@suryans-commit
Copy link
Member Author

/retest

@a-hilaly a-hilaly changed the title Update to aws-sdk-go to v1.50.15 from v1.44.218 Bump aws-sdk-go to v1.50.15 Feb 24, 2024
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

/lgtm
/verify-owners

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2024
Copy link

ack-prow bot commented Feb 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, suryans-commit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot added the approved label Feb 24, 2024
@a-hilaly
Copy link
Member

This is great, thank you @suryans-commit !

@ack-prow ack-prow bot merged commit 9beb5a0 into aws-controllers-k8s:main Feb 24, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants