-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create TES Basespace Upload manager service #186
Conversation
Made a quick go through, looking good. Will go through full review once ready. |
eb1be50
to
5e13dd4
Compare
@victorskl would you mind triggering a srm sequencestatechange in dev and seeing if that triggers this AWS Step function? Might need to be more specific on the filter too (so just to when the statechange is pending analysis?). I would expect the dev step function to fail because it might not be able to generate the v2 samplesheet but just want to make sure that the events are talking. I have deployed stack manually in dev but also added to orcabus pipeline. Will destroy manual dev stack before merging. |
will do |
Gah @williamputraintan is #231 the last of the refactoring? |
(I knew it.! Hang in there, Alexis) |
Yes, no more refactoring! |
Will; let pause the refactor activity at this point. I reckon, we really achieved pretty good refactor this far (thank you, again). We shall let it sit the current code base (what we got up to |
5e13dd4
to
143d38b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, Alexis. Minor comments to attend.
...ad/stateless/stacks/bs-runs-upload-manager/deploy/lib/stacks/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
fcf9326
to
a3cc3e4
Compare
...rkload/stateless/stacks/bs-runs-upload-manager/deploy/stacks/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
...rkload/stateless/stacks/bs-runs-upload-manager/deploy/stacks/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
...rkload/stateless/stacks/bs-runs-upload-manager/deploy/stacks/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
...rkload/stateless/stacks/bs-runs-upload-manager/deploy/stacks/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
...ad/stateless/stacks/bs-runs-upload-manager/deploy/constructs/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
lib/workload/stateless/stacks/bs-runs-upload-manager/deploy/constructs/lambda_layer.ts
Outdated
Show resolved
Hide resolved
...rkload/stateless/stacks/bs-runs-upload-manager/deploy/stacks/bs_runs_upload_manager_stack.ts
Outdated
Show resolved
Hide resolved
lib/workload/stateless/stacks/bs-runs-upload-manager/deploy/README.md
Outdated
Show resolved
Hide resolved
Use jwt portal token from secrets maanger
Remove bs access token from stage, new custom id
Filtered event and updated step function inputs to match sequence. Ran `yarn run prettier --write` to update linting for ts files
…us only Updated orcabus pipeline stack collection class
Import path fixes after refactor Deleted blank deployment readme
583507c
to
d5ec8db
Compare
ESLint ignore for beforeBinding command Run prettier --write for bssh tools lambda layer
@victorskl do you want to look over this again, have refactored onto main and cdk synth now working |
Go for it, Alexis. Pls merge whenever you feel ready. These are all moving targets. We can readjust down the track. Let us pushing forwards... |
Woo hoo! One down, five PRs to go |
You should rest and enjoy ur holis... I can/will follow up testing this part meanwhile. Once we got this part up and running, we should get to fastqs - that is pretty good, already.! Thanks for all hard work.! |
constructor(scope: Construct, id: string, props: PythonLambdaLayerConstructProps) { | ||
super(scope, id); | ||
|
||
this.lambda_layer_version_obj = new PythonLayerVersion(this, 'python_lambda_layer', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexiswl Just minor request, pls.
I just notice these as post-mortem review. When you have spare time down the track, could you please follow up refactoring naming convention on these CDK variables. There seems to be mixed use of snake_case
and camelCase
.
ditto - current guidance on TypeScript
https://github.com/umccr/orcabus/blob/main/README.md#typography
This apply to both bsRunsUploadManager
and ICAv2CopyBatchManager
services that have merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And.. separate PRs for these refactor, pls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And.. separate PRs for these refactor, pls.
Ah that's going to cause some annoying merge conflicts, the PythonLayer construct example above will rename the attribute lambda_layer_version_obj to lambdaLayerVersionObj, but the property of the bs runs upload manager stack (lambda_layer_obj will be renamed to lambdaLayerObj),
so the layers attribute in any of the python functions will go from layers: [props.lambda_layer_obj.lambda_layer_version_obj]
to layers: [props.lambdaLayerObj.lambdaLayerVersionObj]
where lambdaLayerObj
comes from one branch and lambdaLayerVersionObj
comes from another branch.
I think this would be easier to all be in one PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sure. In that case, that's fine, Alexis. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, ok to be all in one go, refactor PR, yes.
TODO