-
Notifications
You must be signed in to change notification settings - Fork 3
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
[BIOMAGE-2017] Removed v1 references #267
Conversation
Codecov Report
@@ Coverage Diff @@
## master #267 +/- ##
==========================================
- Coverage 77.72% 77.07% -0.66%
==========================================
Files 19 19
Lines 1625 1605 -20
==========================================
- Hits 1263 1237 -26
- Misses 362 368 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. |
@@ -65,6 +65,7 @@ load_config <- function(development_aws_server, api_version = "v1") { | |||
pod_name = Sys.getenv("K8S_POD_NAME", "local"), | |||
activity_arn = activity_arn, | |||
api_url = paste0("http://api-",sandbox,".api-",sandbox,".svc.cluster.local:3000"), | |||
api_version = "v2", |
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.
should we define this here? wouldn't it be better to define it in the sysdata.R?
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.
Oh, just looked at the context. the config is created here, so it's not that bad.
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.
it's only ever used in this same function to build the SNS topic name. I was considering removing it altogether
pipeline-runner/tests/testthat/test-gem2s-1-download_user_files.R
Outdated
Show resolved
Hide resolved
pipeline-runner/tests/testthat/test-gem2s-1-download_user_files.R
Outdated
Show resolved
Hide resolved
@echo "Installing local runner" | ||
@(cd ./local-runner && npm install) | ||
@echo "Installing R env packages" | ||
@(cd ./pipeline-runner && R -e "renv::restore()") | ||
build: | ||
@(cd ./local-runner && npm run build) | ||
test: | ||
@(cd ./pipeline-runner && R -e "devtools::test()") |
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.
is this working? we had some problems with @cosa65 running the install script.
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.
it works for me but I had most of the deps already installed. We can merge and debug it later
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.
yep, I agree. we solved it by manually installing the specific packages that were failing
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.
can those missing packages be installed also with a command? like R -e "install.packages(...)
Co-authored-by: german <[email protected]>
Co-authored-by: german <[email protected]>
Description
Details
URL to issue
https://biomage.atlassian.net/browse/BIOMAGE-2017
Link to staging deployment URL (or set N/A)
https://ui-ahriman-flaky-warthog.scp-staging.biomage.net/
Links to any PRs or resources related to this PR
Integration test branch
master
Merge checklist
Your changes will be ready for merging after all of the steps below have been completed.
Code updates
Have best practices and ongoing refactors being observed in this PR
Manual/unit testing
Integration testing
You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.
Documentation updates
Optional