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: add aws batch compute and job queue #1489

Closed

Conversation

sachinmalanki
Copy link
Contributor

Description of your changes

Adding AWS Batch Compute Environment(aws_batch_compute_environment) and Job Queue(aws_batch_job_queue)

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

Manually tested as per the guide
Batch compute environment
image

Job Queue
image

@sachinmalanki
Copy link
Contributor Author

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

1 similar comment
@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

1 similar comment
@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

1 similar comment
@sachinmalanki
Copy link
Contributor Author

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@sachinmalanki
Copy link
Contributor Author

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

1 similar comment
@turkenf
Copy link
Collaborator

turkenf commented Sep 23, 2024

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 23, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 24, 2024

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@sachinmalanki
Copy link
Contributor Author

sachinmalanki commented Sep 25, 2024

The test fails because of hardcoding securityGroupIds: and subnets: values. Can I know how to fix this? The computeenvironment crd does not allow subnet and securitygroupid reference.

@mergenci
Copy link
Collaborator

turkenf and others added 20 commits October 23, 2024 16:43
Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Blake R <[email protected]>
@sachinmalanki
Copy link
Contributor Author

Hi @turkenf. Thanks for the suggestions, I have rebased and made the changes. Could you please review ?
When I was testing this locally, the service_role gets deleted first and leaves the computeenvironment in a INVALID hanging state. Any idea how this could be fixed? This could cause the uptest to fail.

@turkenf
Copy link
Collaborator

turkenf commented Oct 24, 2024

When I was testing this locally, the service_role gets deleted first and leaves the computeenvironment in a INVALID hanging state. Any idea how this could be fixed? This could cause the uptest to fail.

@sachinmalanki, we have hooks for this kind of cases. We can use pre-delete-hook. Here is an example: yaml and sh files.

I noticed that there are currently 47 commits in the PR’s commit history. When you have a chance, could you please fork your PR from the latest main branch and squash your commits? It would help keep the history clean. Thanks so much!

Comment on lines 10 to 21
r.References = config.References{
"compute_resources.subnets": config.Reference{
TerraformName: "aws_subnet",
RefFieldName: "SubnetRefs",
SelectorFieldName: "SubnetSelector",
},
"compute_resources.security_group_ids": config.Reference{
TerraformName: "aws_security_group",
RefFieldName: "SecurityGroupRefs",
SelectorFieldName: "SecurityGroupSelector",
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
r.References = config.References{
"compute_resources.subnets": config.Reference{
TerraformName: "aws_subnet",
RefFieldName: "SubnetRefs",
SelectorFieldName: "SubnetSelector",
},
"compute_resources.security_group_ids": config.Reference{
TerraformName: "aws_security_group",
RefFieldName: "SecurityGroupRefs",
SelectorFieldName: "SecurityGroupSelector",
},
}
r.References["compute_resources.subnets"] = config.Reference{
TerraformName: "aws_subnet",
RefFieldName: "SubnetRefs",
SelectorFieldName: "SubnetSelector",
}
r.References["compute_resources.security_group_ids"] = config.Reference{
TerraformName: "aws_security_group",
RefFieldName: "SecurityGroupRefs",
SelectorFieldName: "SecurityGroupSelector",
}

@sachinmalanki
Copy link
Contributor Author

I have raised another PR - #1542 with all the changes suggested.

@turkenf
Copy link
Collaborator

turkenf commented Oct 25, 2024

Hi @sachinmalanki, which PR will we continue with?

@sachinmalanki
Copy link
Contributor Author

Lets continue with #1542. let me close this PR

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.

8 participants