Skip to content
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

Display engine version for Nextflow workflows #2001

Merged
merged 7 commits into from
Aug 14, 2024

Conversation

kathy-t
Copy link
Contributor

@kathy-t kathy-t commented Aug 7, 2024

Description
This PR displays the Engine Versions column instead of Language Versions for Nextflow workflows.

Screenshot 2024-08-07 at 10-53-16 Dockstore Workflow github com_nf-core_rnaseq

Review Instructions
On QA, navigate to https://qa.dockstore.org/workflows/github.com/nf-core/rnaseq:dev?tab=versions and verify that there's an Engine Versions column and no Language Versions column for Nextflow workflows. Go to a non-Nextflow workflow, click the Versions tab and verify that it has a Language Versions column.

Issue
dockstore/dockstore#5918
https://ucsc-cgl.atlassian.net/browse/DOCK-2536

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that your code compiles by running npm run build
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If this is the first time you're submitting a PR or even if you just need a refresher, consider reviewing our style guide
  • Do not bypass Angular sanitization (bypassSecurityTrustHtml, etc.), or justify why you need to do so
  • If displaying markdown, use the markdown-wrapper component, which does extra sanitization
  • Do not use cookies, although this may change in the future
  • Run npm audit and ensure you are not introducing new vulnerabilities
  • Do due diligence on new 3rd party libraries, checking for CVEs
  • Don't allow user-uploaded images to be served from the Dockstore domain
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.
  • Check whether this PR disables tests. If it legitimately needs to disable a test, create a new ticket to re-enable it in a specific milestone.

@kathy-t kathy-t self-assigned this Aug 7, 2024
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.69%. Comparing base (e6b5784) to head (ecae2d4).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2001      +/-   ##
===========================================
+ Coverage    41.67%   41.69%   +0.01%     
===========================================
  Files          393      393              
  Lines        12307    12312       +5     
  Branches      2955     2957       +2     
===========================================
+ Hits          5129     5133       +4     
  Misses        4854     4854              
- Partials      2324     2325       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

</th>
<td mat-cell *matCellDef="let version">
<div *ngFor="let engineVersion of version.versionMetadata?.engineVersions">
<div class="no-wrap">{{ engineVersion }}</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this no-wrap otherwise it wraps the nextflow engine version in an unfriendly way like below

Screenshot 2024-08-07 at 11-35-01 Dockstore Workflow github com_nf-core_rnaseq

When there are multiple engine versions, they are displayed one per line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the repetition and because we're used to seeing the most significant part of a phrase on the left of text (in left-to-right languages), I wonder if we should strip Nextflow for the version column values, e.g., just show !>=22.10.1 instead of Nextflow !>=22.10.1. It would also make the table a little less crowded.

I think it still makes sense to have the full string, with the Nextflow word on the facets page, as there are all types of workflows on the search page, whereas for this table the column is only visible on a Nextflow workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems intentional that we're storing the engine name based on this comment

https://github.com/dockstore/dockstore/blob/ee8503a70285afdc8fcfdb70580c59cdf8130889/dockstore-webservice/src/main/java/io/dockstore/webservice/languages/NextflowHandler.java#L492-L494

Proposing we leave it as is for now and create a new ticket if it becomes an issue since it'll require a webservice change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, citing my own comment against my argument. :)

I'd still argue that it's redundant on this page for Nextflow. My suggestion, which wasn't clear, would be to have a pipe here, not to change it on the backend -- I think it still makes sense to have Nextflow !>=22.10.1 on the facets page for differentiation when we add other engines for other languages. But here it's already a Nextflow workflow, and there are no other engines for Nextflow other than Nextflow itself, so it's not really adding anything and slightly getting in the way of viewing the significant info.

Copy link
Member

@denis-yuen denis-yuen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshots look good

</th>
<td mat-cell *matCellDef="let version">
<div *ngFor="let engineVersion of version.versionMetadata?.engineVersions">
<div class="no-wrap">{{ engineVersion }}</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the repetition and because we're used to seeing the most significant part of a phrase on the left of text (in left-to-right languages), I wonder if we should strip Nextflow for the version column values, e.g., just show !>=22.10.1 instead of Nextflow !>=22.10.1. It would also make the table a little less crowded.

I think it still makes sense to have the full string, with the Nextflow word on the facets page, as there are all types of workflows on the search page, whereas for this table the column is only visible on a Nextflow workflow.

src/app/workflow/versions/versions.component.html Outdated Show resolved Hide resolved
@kathy-t kathy-t requested a review from coverbeck August 13, 2024 14:20
Copy link
Contributor

@coverbeck coverbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my comment, I still feel we should drop Nextflow, but I don't feel strongly enough to block, and if you still think we should keep it, I'm good with that.

</th>
<td mat-cell *matCellDef="let version">
<div *ngFor="let engineVersion of version.versionMetadata?.engineVersions">
<div class="no-wrap">{{ engineVersion }}</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, citing my own comment against my argument. :)

I'd still argue that it's redundant on this page for Nextflow. My suggestion, which wasn't clear, would be to have a pipe here, not to change it on the backend -- I think it still makes sense to have Nextflow !>=22.10.1 on the facets page for differentiation when we add other engines for other languages. But here it's already a Nextflow workflow, and there are no other engines for Nextflow other than Nextflow itself, so it's not really adding anything and slightly getting in the way of viewing the significant info.

@kathy-t
Copy link
Contributor Author

kathy-t commented Aug 13, 2024

Updated so that 'Nextflow' is removed from the engine version value for the Versions tab.

image

@kathy-t kathy-t requested a review from coverbeck August 13, 2024 19:29
Copy link

sonarcloud bot commented Aug 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@kathy-t kathy-t merged commit 255099a into develop Aug 14, 2024
18 of 19 checks passed
@kathy-t kathy-t deleted the feature/5918/nf-engine-version branch August 14, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants