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

Modifies the behavior of the tag_filter #165

Closed
wants to merge 2 commits into from
Closed

Modifies the behavior of the tag_filter #165

wants to merge 2 commits into from

Conversation

sabjorn
Copy link

@sabjorn sabjorn commented Jan 7, 2018

Now, if branch is not set tags from all branches will be searched.

This is accomplished through disabling the --single-branch git flag from the check and in assets whenever the branch configuration flag is set by the user.

Two tests were added to check.sh. One is responsible for making sure all tags on all branches are available to the filter (it_can_check_with_tag_filter_for_all_branches). One is responsible to make sure tags are ignored from other branches when a branch is specified (it_can_check_with_tag_filter_for_all_branches).

Additionally:

  • Small modifications to helpers.sh to facilitate new tests.
  • A git flag, --merged, was added to the check asset any time the --single-branch flag is present. This is because all tags are present on all branches for locally cloned git repos (like this present in the tests).
  • the $tag_filter if statement in the check resource was modified by replacing repeating commands with a environment variable. This was done to reduce errors.

…t tags from all branches will be searched.

This is accomplished through disabling the `--single-branch` git flag from the `check` and `in` assets whenever the `branch` configuration flag is set by the user.

Two tests were added to `check.sh`. One is responsible for making sure all tags on all branches are available to the filter (`it_can_check_with_tag_filter_for_all_branches`). One is responsible to make sure tags are ignored from other branches when a branch is specified (`it_can_check_with_tag_filter_for_all_branches`).

Additionally:
* Small modifications to `helpers.sh` to facilitate new tests.
* A git flag, `--merged`, was added to the `check` asset any time the `--single-branch` flag is present. This is because all tags are present on all branches for locally cloned git repos (like this present in the tests).
* the `$tag_filter` if statement in the `check` resource was modified by replacing repeating commands with a environment variable. This was done to reduce errors.
@sabjorn
Copy link
Author

sabjorn commented Jan 10, 2018

@vito so it looks like this isn't going to work either.
I made a test pipeline and a mock git repo and there seems to be some trouble still.

It all seems to be related to:

git_tag_base='git tag --list "$tag_filter" --sort=creatordate $mergedflag'
if [ -n "$ref" ]; then
      eval $git_tag_base --contains $ref

(lines 81-83 in assets/check)

If I remove this and simply have:

git_tag_base='git tag --list "$tag_filter" --sort=creatordate $mergedflag'
eval $git_tag_base | tail -1

It works (although test it_can_check_with_tag_filter_with_cursor in tests/check.sh fails).

In the README it states:

The repository is cloned (or pulled if already present), and any commits from the given version on are returned.

My assumption is that the troubles i'm facing are related to the "pulled if already present" part.

Any ideas?

@vito
Copy link
Member

vito commented Jan 11, 2018

Seems more likely to be due to the --contains bit. This flag is used to ensure that, when checking from a given version (i.e. it's not the first check ever run), it returns all tags after the given commit (including the commit's own tag). This is to fit the "correct" Concourse resource check semantics.

This assumes that a given tag N will have all tags N-1, N-2, etc. prior to it in the commit history, and that tags aren't dangling on their own HEADs. Could this be what's failing in your scenario?

@sabjorn
Copy link
Author

sabjorn commented Jan 18, 2018

@vito I think this is what is causing the failure. When the git repo is pulled without a branch, it is master which is pulled. Thus, ref is the commit ID on master. So, when --contains is run on this, it ignores all other branches.

So, for example, if I have a repo with tags:

version-0.0.1-rc1 (on `master`)
version-0.0.2-rc1 (on `first_branch`)
version-0.0.3-rc1 (on `second_branch`)
version-0.0.4-rc1 (on `second_branch`)

when git tag --list "$tag_filter" --sort=creatordate --contains $ref runs, it only gets version-0.0.1-rc1. So, only the tags contained on master.

Any ideas?

Also, I will admit that it is very possible I am wrong.

@vito
Copy link
Member

vito commented Jan 23, 2018

I'd be surprised if that's how it behaves; tags point directly to trees and exist across all branches. If version-0.0.2-rc1 points to a tree that has the --contains ref anywhere in its ancestry, it should be listed.

@sabjorn
Copy link
Author

sabjorn commented Jan 23, 2018

I'm not sure if that is completely accurate.

For example, if you look at the --merged flag for tags. Using this flag will have the same output as the behaviour noted in my previous comment.

Basically, it appears that tags are actually attached to specific commits, and if that commit does not exist in the history of the branch, it will not exist in the list when running git tags --list.

So, it would follow (I think...), if first_branch has a tag version-0.0.2-rc1 but you run git tag --list "version-*" --contains [current mater head] you will only get version-0.0.1-rc1 because the commit on the other branch is not in the history of master.

@vito
Copy link
Member

vito commented Jan 24, 2018

@sabjorn Well, if the tags are on completely different branches not sharing a given common ref, that'd explain it too - not because the tag listing is scoped to a branch, but because the ancestry isn't shared, right? (i.e. the other tags cannot --contain the ref)

Currently tag_filter expects the tags to be rolling forward on a shared ancestry, but if they're all on distinct trees of the repo, there's no longer anything tying them together sequentially, making it harder for the resource to do its job with /check. The only thing to order them by would then be their names, which is messy since they're not currently expected to be anything but "something that matches the filter" - i.e. not assumed to be semver. It could also be confusing when new patch versions are created for previous minor versions.

@sabjorn
Copy link
Author

sabjorn commented Jan 24, 2018

In the context of this global tag_filter, won't --sort=creatordate be sufficient?

For more context: the reason I need this change is that my team uses new branches for releases, so changing the CI for each release would be necessary. However, I have different tag filters for the different types of triggers (e.g. version-*-rc* vs version-*-release). So, the most recent tag is all i'm interested in.

I've been running a modified version of this PR (with the --contains $ref section commented) for a few weeks and have no problems.

@vito
Copy link
Member

vito commented Jan 25, 2018

@sabjorn True! I'd be fine with removing --contains, but per the resource interface, it should only include the tags from after the given ref, including it. If we just remove --contains, it'll return all of them, which is incorrect.

So, if versions v1, v2, v3 exist, and I check from v2, it should only return v2, v3.

I think in the case of tag_filter, the returned ref is already the tag, so this might just take a loop to walk over the tags and exclude the ones from before the given ref. Note that there's an edge case where the given ref isn't there at all, in which case it should return the latest one. Does that make sense?

@sabjorn
Copy link
Author

sabjorn commented Feb 21, 2018

@vito finally got back to working on this a bit.
I think maybe the problem is that $ref doesn't actually point to the tag. The tag filter is only referenced with a filter later.
When the check script spins up, if the destination exists, it runs

git fetch
git reset --hard FETCH_HEAD

Unless i'm reading this incorrectly, this means $ref is the tip of FETCH_HEAD. So, when a branch isn't specified, $ref will always be the latest commit on master and so --contains $ref will not be able to return any commits that are not merged into master. This is sort of a repeat of what I posted above (apologies). So, here is the problem:

If $ref exists, but the tag we're searching for is off on some other branch unmerged, the consideration of tags only after the current reference falls apart. By this I mean, if $ref points to something that does not actually have the tags we're looking for, then there is no basis from which to say "these tags are after the given ref".

At this point, i'm thinking that probably the best idea is to modify:

    if [ -n "$ref" ]; then
      git tag --list "$tag_filter" --sort=creatordate $mergedflag --contains $ref

So that it makes sure it is also on a branch to run the --contains $ref flag:

    if [ -n "$ref" ] && [ -n "$branch" ] ; then
      git tag --list "$tag_filter" --sort=creatordate $mergedflag --contains $ref

This way, if we're on a branch, $ref will be valid otherwise it wont be considered.


However, maybe i'm getting something wrong. If the $destination exists, does this mean it will be on the commit from the previous IN action?

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.

2 participants