-
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
STG Deployment #56
STG Deployment #56
Conversation
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. Thanks for expediting this deployment task.
Just few minor comments and pointers. I let you decide whether take action now or skip them. Happy either way.
.gitignore
Outdated
@@ -17,3 +17,6 @@ cfnnag_output | |||
/sample_file/ | |||
/src/layers/umccr_utils/python/ | |||
.aws-sam | |||
|
|||
.yarn |
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.
Good move to localised yarn v4, Will. This git ignore looks a bit wide.
The localised yarn (v4 or latest) script has to check it into the repo. Hence, localised.
There should be couple of folder need exclusion from git ignore: e.g. !.yarn/releases
(where localised yarn script installed) and !.yarn/plugins
(any extensions use). So on so forth.
See Portal example:
- https://github.com/umccr/data-portal-apis/tree/dev/.yarn
- https://github.com/umccr/data-portal-apis/blob/dev/.gitignore#L45-L52
And here is pointer to doc:
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.
Ahh thanks for this! Will update it
@@ -0,0 +1 @@ | |||
nodeLinker: node-modules |
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.
If we do ^^ git ignore changes on .yarn
part then this .yarnrc.yml
config should change a little bit more to reflect the localised yarn situation.
For example, it will contain pointer to localised yarn path.
yarnPath: .yarn/releases/yarn-4.3.0.cjs
See also:
https://github.com/umccr/data-portal-apis/blob/dev/.yarnrc.yml
Makefile
Outdated
@cdk synth | ||
install: | ||
@corepack enable | ||
@yarn set version stable |
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.
If we localised (frozen) it well, we may rearrange corepack enable
and yarn set version stable
to another Makefile target (such as yarn-deps-bump) as we may not want to pull in latest yarn with every other CI build. Instead we inform CI environment (and other developer local dev) to leverage the localised yarn version from the repo.
We run these yarn set version stable
in another time when we wish to bump yarn version itself in controlled dependencies upgrade manner.
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, so I think the set yarn version is pinned based on the packageManager
field on the package.json
file. It seems that this is the new recommended way of installing yarn rather than the local binary checked in the to repo from the v4 installation guide: https://yarnpkg.com/blog/release/4.0#installing-yarn
But do agree that the install target should only be install commands instead of any env setup, so this could move to another makefile target.
--form "file=@${SAMPLESHEET_FILE}" \ | ||
"${API_URL}" | ||
``` | ||
|
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.
This README test section still applicable? Or perhaps, you have other plan with it or rearranging/refactoring it? 🙄
Especially this last section help me how to leverage sscheck API endpoint, you know. Perhaps about time create /docs
etc.
If you have a plan, that's ok with this changes. We can always dig to git history, if any.
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.
Yeah could maybe put this to docs
instead of removal. Well it is because I put in local invoke via sam
cli, and so testing locally could be done via the curl API instead of this script. The local invoke API could be configured so that it will run locally but could use the remote metadata endpoint with just make start
(and use the curl/postman command to check the local samplesheet file)
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.
Yup. Developer guide | User guide would be handy, Will.
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.
Taking a bit to understand the code. LGTM!
Resolves #55
Todo:
stg
branch fromdev