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

Adding a gem.bat to fix the ruby.exe path issue #636

Closed
wants to merge 1 commit into from

Conversation

cdenneen
Copy link
Contributor

No description provided.

@cwjohnston
Copy link
Contributor

We've merged changes in sensu/sensu-omnibus#203 that I believe will make it unnecessary for puppet to directly manage this file. I've kicked off builds for 0.29.0-9 packages so we may test that theory.

@cdenneen
Copy link
Contributor Author

cdenneen commented Apr 13, 2017

@cwjohnston great news but I'm slightly confused.

  • you didn't want to move to gem.cmd because this would break installs for versions <0.27
  • without doing this change it will only fix versions >0.29

So this fix will work with all older versions... and would still work with 0.29
At the point that you've decided to deprecate older than 0.27, the bat file wouldn't be necessary and the cmd PR #616 would work for >0.27

I knew you wouldn't go back and release older versions updating the bat file so figured this was a better solution to cover all scenarios... you're call obviously.

@cwjohnston
Copy link
Contributor

you didn't want to move to gem.cmd because this would break installs for versions <0.27

To be clear, I'm not fully 👎 on using gem.cmd over gem.bat, I am merely calling out risks I see and trying figure out what will cause the least confusion.

To me there's still an open question of how gem.cmd came to be part of the package build. I believe it was added by upstream Ruby when we moved from 2.0 to 2.3.0 circa Sensu 0.27. As I continue to look into this I see that gem.cmd is present in 0.27, 0.28 and 0.29 packages and isn't being explicitly added by our omnibus build toolchain. I think we can merge #616.

So this fix will work with all older versions... and would still work with 0.29

I am reluctant to endorse unconditionally overwriting a file provided by official packages, as it adds a corner case we have to consider should changes to that aspect of packaging be needed in the future.

I knew you wouldn't go back and release older versions updating the bat file so figured this was a better solution to cover all scenarios...

I agree, we won't be back-porting a fix and releasing new packages for these older versions.

@cdenneen
Copy link
Contributor Author

cdenneen commented Apr 13, 2017

@cwjohnston ok whatever the internal team feels is best...
#616 will break versions older than 0.27 as you've pointed out.
Yes this creates a corner case but until < 0.27 are officially deprecated from support I think it might become a necessary evil in order to have a module that works in those cases.

I see 3 options:

1. Officially stop supporting versions < 0.27

2. Support older versions

3. Drop support for all versions < 0.29.0-9

  • No PR merge necessary

@jaxxstorm @hprins

@cwjohnston
Copy link
Contributor

I see 3 options

I agree with your analysis. I've merged #616 to address this.

@cwjohnston cwjohnston closed this Apr 18, 2017
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