-
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
21.04.1 List of changes done by Lifebit #19
base: 21.04.1-original
Are you sure you want to change the base?
Conversation
* fix: Lustre recursive copy support * chore: add missing changes for batch with lustre * fix: Error of SSL certificate when running Jobs with Lustre | CI-213 * fix: update copy command evaluation Co-authored-by: rubengomex <ruben@lifebit.ai> Co-authored-by: RaduP <radu@lifebit.ai>
…1-gitlab-fixes # Conflicts: # modules/nf-commons/src/main/nextflow/Const.groovy
fix: Authorization header to enable oauth token for gitlab
* feat: 21.04.1-update-to-use-imdsv2 * feat: return IAM role not ARN
@@ -262,6 +256,13 @@ class BashWrapperBuilder { | |||
binding.launch_cmd = getLaunchCommand(interpreter,env) | |||
binding.stage_cmd = getStageCommand() | |||
binding.unstage_cmd = getUnstageCommand() | |||
|
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 dont think this change make sense. so dropping it
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.
What change did you drop? then, I'm siging the stagingScript prop being populated as well. This is needed because .getStageInputFilesScript can actually be fetching the files from s3 or directly from lustre mounted file system.
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.
@rubengomex I think only but line 263-23 is moved from 254-255, but this line (281) is newly added. that is considered for development
@@ -58,12 +58,12 @@ class Const { | |||
/** |
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.
not sure whether this is needed
@@ -39,11 +39,12 @@ dependencies { | |||
compileOnly 'org.pf4j:pf4j:3.4.1' | |||
|
|||
compile ('io.nextflow:nxf-s3fs:1.1.0') { transitive = false } | |||
compile ('com.amazonaws:aws-java-sdk-s3:1.11.542') |
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.
already packages are using 1.12.129
feat: fix .command.run upload issue for LP-5254
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 changes done by @mageshwaran-lifebit to make pipeline test rans
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 changes done by @mageshwaran-lifebit
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 changes done by @mageshwaran-lifebit to make pipeline test rans
@@ -251,6 +253,20 @@ class AwsBatchExecutor extends Executor implements ExtensionPoint { | |||
@PackageScope | |||
ThrottlingExecutor getReaper() { reaper } | |||
|
|||
String getInstanceIdByQueueAndTaskArn(String queue, String taskArn) { |
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 getInstanceIdByQueueAndTaskArn
function is not required, as we are moving towards nf-monitor plugin
@@ -132,37 +136,21 @@ class AmazonClientFactory { | |||
* The IAM role name associated to this instance or {@code null} if no role is defined or | |||
* it's not a EC2 instance | |||
*/ | |||
protected String fetchIamRole() { | |||
private String fetchIamRole() { |
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.
nf-amazon-1.12.0 already have this
@@ -171,12 +159,11 @@ class AmazonClientFactory { | |||
* The AWS region of the current EC2 instance eg. {@code eu-west-1} or | |||
* {@code null} if it's not an EC2 instance. | |||
*/ | |||
protected String fetchRegion() { | |||
private String fetchRegion() { |
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.
nf-amazon-1.12.0 already this
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.
we dont need this file in newer version
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.
No description provided.