Skip to content

Support for Git tags #191#192

Merged
stain merged 3 commits intocommon-workflow-language:masterfrom
coverbeck:gittag_support
May 22, 2018
Merged

Support for Git tags #191#192
stain merged 3 commits intocommon-workflow-language:masterfrom
coverbeck:gittag_support

Conversation

@coverbeck
Copy link

@coverbeck coverbeck commented Mar 12, 2018

Description

Unless it was as commit id, GitService.java was always specifying /refs/remotes/origin/<branch> for the name to checkout. This does not work with Git tags. See Stack Overflow. See if there is a If a RefNotFoundException fetching a branch, and if there is, try with the name only. If that also gives an error, thrown the original exception.

Motivation and Context

This is to allow users to visualize CWL files that referenced by a Git tag. I ran into this doing the integration with Dockstore.org and CWLViewer.

Fixes #191

How Has This Been Tested?

I tested this manually, by running CWLViewer locally and entering workflows that had both branches and tags in their paths.

There are no existing unit tests in this area, and I did not add any new tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

If we get a RefNotFoundException, it might be a tag.
mr-c
mr-c previously requested changes Apr 15, 2018
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Looks great, can we get a test for this fix?

Charles Overbeck and others added 2 commits April 23, 2018 17:13
Had to introduce mockito as a (test) dependency.

Refactored GitService a little to allow for mocks.
@stain stain dismissed mr-c’s stale review May 22, 2018 13:47

Tests were added as requested

@stain stain self-requested a review May 22, 2018 13:47
Copy link
Member

@stain stain left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good!

repo.checkout()
.setName(branchOrCommitId)
.call();
try {
Copy link
Member

Choose a reason for hiding this comment

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

Agree on the logic of trying branch before tag, that's the logic used at GitHub as well - see https://github.com/stain/branchandtagtest/blob/test/README.md where I tried to make a branch and a tag both called test (but the branch further ahead)

@stain stain merged commit 4ce9ffa into common-workflow-language:master May 22, 2018
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.

Viewer Does Not Work Against Git Tags

3 participants