toil icon indicating copy to clipboard operation
toil copied to clipboard
verified publisher icon DataBiosphere

Issues/3514 missing permission warning aws

Open Hexotical opened this issue 3 years ago • 2 comments

Changelog Entry

To be copied to the draft changelog by merger:

Add code for warning check to be used when launching cluster with AWS.

Reviewer Checklist

  • [ ] Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
    • [ ] If it is coming from an external repo, make sure to pull it in for CI with: 
      contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
    • [ ] If there is no associated issue, create one.
  • [ ] Read through the code changes. Make sure that it doesn't have:
    • [ ] Addition of trailing whitespace.
    • [ ] New variable or member names in camelCase that want to be in snake_case.
    • [ ] New functions without type hints.
    • [ ] New functions or classes without informative docstrings.
    • [ ] Changes to semantics not reflected in the relevant docstrings.
    • [ ] New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
    • [ ] New features without tests.
  • [ ] Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
  • [ ] Finish the review with an overall description of your opinion.

Merger Checklist

  • [ ] Make sure the PR passes tests.
  • [ ] Make sure the PR has been reviewed since its last modification. If not, review it.
  • [ ] Merge with the Github "Squash and merge" feature.
    • [ ] If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
  • [ ] Copy its recommended changelog entry to the Draft Changelog.
  • [ ] Append the issue number in parentheses to the changelog entry.

Hexotical avatar Apr 27 '22 21:04 Hexotical

I am still due to re-review this.

I think the CI failures are #4168 so those aren't really the fault of this PR.

adamnovak avatar Jul 26 '22 22:07 adamnovak

@Hexotical I think you have the argument order wrong when you call client AKA get_client and the AWS service name and zone are being swapped. The CI tests that launch clusters seem to be failing to do so.

Can you fix that and launch a cluster manually to confirm it actually works?

adamnovak avatar Aug 10 '22 17:08 adamnovak

It looks like some CWL conformance tests timed out when running against Kuberentes. This PR shouldn't actually affect that; I think maybe our Kubernetes was too busy and our tests can't handle that happening.

https://ucsc-ci.com/databiosphere/toil/-/jobs/20491#L2558

I am going to rerun the offending tests.

adamnovak avatar Aug 19 '22 20:08 adamnovak