-
Notifications
You must be signed in to change notification settings - Fork 54
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
Correct RedHad alternatives and profile.d handling #31
base: master
Are you sure you want to change the base?
Conversation
FIXES: Manipulating symlinks directly on RedHad hosts without using alternatives leaves the host in an inconsistent state. In addition, JAVA_HOME was not being set for sh shells.
Thanks for the PR. It seems like a better approach to use the alternatives command. Could you please update the tests that are failing in TravisCI? (see Details link below) Change the test from the old To test your new resource: Thanks, -Tyler |
I'm afraid I don't know what TravisCI is and I don't have liberty to research this for you. FICO's licensing obligation in this project appears to have been satisfied when I documented and published our material change in the original post to you per the license that you attached to this project. Following your additional instructions to fork the project, etc, was a gesture of good faith to help you integrate the change. Please take the ball from here to satisfy your TravisCI requirement. |
Travis CI is an automated testing service, commonly used for open source projects. If you are viewing this conversation through GitHub, it shows up as a yellow box at the bottom of the thread that says "Failed - The Travis CI build failed". The reason why it failed is that your updated broke one of the rspec unit tests. It is a good process to update unit tests along with feature commits. I don't see how this is related to licensing concerns at FICO. The purpose of unit tests is to ensure quality and to detect when incoming changes may break other features. In this case it has detected that your changes break the test to ensure that a link was created, because we are testing for the declaration of a link file. Since you are now doing this with an Exec call, then the test just needs to be updated, and I included some sample snippets above on how to do this. The test file is in spec/classes/jdk_oracle_init_spec.rb If you are not interested in in having your changeset not break the build, then I can take over from here and update the tests, but it might be a while. My current work project has nothing to do with Java, and nobody else has mentioned any issue with using links vs alternatives command, so I will most likely wait until there is a clear reason to go forward with your new changes and update the unit tests to match, before merging it into master. |
In the time it took you to write all that, you probably could have just fixed your unit test, which you are far more qualified than me to update. Your decision to do otherwise is regrettable. Compliance with RHEL best-practices should be everyone's concern and make no mistake, stomping all over alternatives with externally-managed symlinks is certainly not best-practice. Failing to set JAVA_HOME on a machine with Oracle JDK is also clearly not best-practice. I agree that unit tests are a widely adopted best-practice, but I just don't know your test infrastructure, so please: take the ball from here. I don't have the luxury of experimentation with this to learn it and get it right for you. Providing our material alteration of your open source code was again, a gesture of good faith in adhering to your license terms. The rest is up to you. |
I pushed a commit to fix the rspec tests and clarify the contribution process for future PRs, but I have not tested this yet. If another person could comment to confirm that they have tested against this branch so we have another OS covered then I'll merge it in and ship another release to the forge. |
Thanks for testing. Lets come back to this after the other merges are sorted out |
+1 on this PR. If there's any testing I can do to help get this through, let me know. |
+1 on this PR. For some reason the redhat code manages the symlinks directly, and also puts them in /usr/sbin (??) instead of /usr/bin. The Debian code in that same file (init.pp) would work perfectly for RedHat, not sure why RedHat has this special case. |
FIXES: Manipulating symlinks directly on RedHad hosts without using
alternatives leaves the host in an inconsistent state. In addition,
JAVA_HOME was not being set for sh shells.