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

Support for Git tags #191 #192

Merged
merged 3 commits into from
May 22, 2018

Conversation

coverbeck
Copy link
Contributor

@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?

coverbeck 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
This pull request was closed.
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