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

Add version check for minor pin #365

Closed
wants to merge 1 commit into from

Conversation

ralucasg
Copy link
Contributor

@ralucasg ralucasg commented Apr 3, 2018

No description provided.

Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

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

@ralucasg This is the right idea -- it can be simplified down to:

{% if salt_release.split('.')|length >= 3 %}
{% set salt_release = 'archive/' ~ salt_release %}
{% endif %}

So just fixing the condition and the Jinja concatenation operator (~ instead of +).

myii added a commit to myii/salt-formula that referenced this pull request Jan 24, 2019
…mulas#364)

* All minor releases appear in an `archive` sub-directory.
* Resolves main bug in GitHub issue saltstack-formulas#364.
* Simplification of original GitHub PR saltstack-formulas#365 by @ralucasg.
@ralucasg
Copy link
Contributor Author

@myii I remember testing it against some corner cases and getting some failures. Sadly it did not cross my mind to write it down and I don't quite remember when it was breaking. It might have been on the "Pin to Major Release" option since those links don't have 'archive/' in them or maybe on something else. I'm truly sorry I did not add my test results as a comment on the PR.

@myii
Copy link
Member

myii commented Jan 25, 2019

@myii I remember testing it against some corner cases and getting some failures. Sadly it did not cross my mind to write it down and I don't quite remember when it was breaking. It might have been on the "Pin to Major Release" option since those links don't have 'archive/' in them or maybe on something else. I'm truly sorry I did not add my test results as a comment on the PR.

@ralucasg Thanks for your response. The edge case was due to:

{% if check_version|length > 1 %}

That would end up being applied for minor and major releases. Changing that to > 2 would be enough or like I did >= 3.

More importantly, I always prefer credit going to the person who has attempted to solve the problem. Your solution was virtually there and I'd be happy to to switch out my PR #394 to revive your PR #365 again. Or another way that could be done is that I supply your details to the commit in #394 so that the authorship goes back to you, which I can do by:

$ git commit --amend --author="Your Name <you@example.com>"

Let me know how you would like to proceed. If you're not too bothered, we can just leave #394 as-is.

@myii
Copy link
Member

myii commented Jan 25, 2019

@ralucasg Sorry, I don't mean for you to send your e-mail address here in GitHub. In fact, I just realised I can use the anonymous GitHub-based address:

I'll just do that and amend #394.

myii pushed a commit to myii/salt-formula that referenced this pull request Jan 25, 2019
…mulas#364)

* All minor releases appear in an `archive` sub-directory.
* Resolves main bug in GitHub issue saltstack-formulas#364.
* Simplification of original GitHub PR saltstack-formulas#365 by @ralucasg.
@myii
Copy link
Member

myii commented Jan 25, 2019

@ralucasg OK, done. If you see #394, you'll see the commit has your authorship. Once the merge is complete, you'll be recognised as a contributor to this project. Thank you for your bug report and your PR.

@ralucasg
Copy link
Contributor Author

@myii Thank you so much, you didn't have to go that far as you added the correct solution and tested it so I believe you deserve the credits more than me.
Also thank you for fixing the formula.

@myii
Copy link
Member

myii commented Jan 25, 2019

@ralucasg You're welcome.

... as you added the correct solution and tested it so I believe you deserve the credits more than me.

Actually, it doesn't work like that. If you had left this PR open, then this would have been the process:

  1. Someone would have reviewed your code and suggested improvements or even the solution.
  2. You would have made the changes and pushed them to this PR.
  3. The PR would be merged, with the entire credit given to you.

So we've just recreated that here -- nothing extra.

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