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

Use full path in the Gemfile #15503

Merged
merged 1 commit into from
Nov 6, 2017
Merged

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jul 3, 2017

Relative pathing can have problems if the pwd is not the manageiq
directory. This ensures the correct path.

Related:

@NickLaMuro Please review
cc @mkanoor @himdel

@Fryguy
Copy link
Member Author

Fryguy commented Jul 3, 2017

We shouldn't use relative paths in the Gemfile, hence why I made this PR.

However, I don't think this "fixes" the --no-deployment nonsense, because gem references are

saved as relative in Gemfile.lock (even when absolute in Gemfile)

Combine that with the double-bundle-install in the plugin repos [1], and it will still fail. We still need to get rid of the double-bundle-install. cc @bdunne @durandom on this bit.

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@NickLaMuro
Copy link
Member

@Fryguy This will not work, and I did already try: ManageIQ/manageiq-ui-classic#1606 (comment)

Bundler, as far as I can tell, will evaluate and save Bundler::Source::Path's as relative paths, because when a Gemfile is saved, you don't want to have an absolute path saved into your Gemfile.lock and checked into source control.

The bug that I brought up to bundler, rubygems/bundler#5817, does attempt to solve this to some degree, but I think the other bug that might exist is the evaluating the Gemfile and the Gemfile.lock (which the two get compared when doing a bundle install --deployment) causes different paths to be evaluated when dealing with eval_gemfile (I have yet to confirm).

An easy way to test that this doesn't work is to check this branch out, symlink manageiq into the manageiq-ui-classic's spec/manageiq directory, and run steps outlined by @himdel in ManageIQ/manageiq-ui-classic#1606 's PR description (the rm Gemfile.lock vendor... ; bundle ; bundle bit). I am pretty sure that it will not work.

@Fryguy
Copy link
Member Author

Fryguy commented Jul 5, 2017

This will not work

I know...

However, I don't think this "fixes" the --no-deployment nonsense

My argument is we should never have relative paths in the Gemfile, regardless.

@NickLaMuro
Copy link
Member

NickLaMuro commented Jul 5, 2017

My apologies, I missed the comment under the PR description. I guess you can ignore my comment. Fine with this being more verbose and exact if that was the intent.

@miq-bot
Copy link
Member

miq-bot commented Jul 5, 2017

This pull request is not mergeable. Please rebase and repush.

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

Please rebase...

Relative pathing can have problems if the pwd is not the manageiq
directory.  This ensures the correct path.

Related:
- ManageIQ#14525
- ManageIQ/manageiq-ui-classic#1606
- ManageIQ/manageiq-content#136
@Fryguy Fryguy force-pushed the expand_path_mime_types branch from 1a8c47e to b78a934 Compare November 6, 2017 20:07
@Fryguy
Copy link
Member Author

Fryguy commented Nov 6, 2017

@bdunne Updated

@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2017

Checked commit Fryguy@b78a934 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@bdunne bdunne merged commit 6881d5a into ManageIQ:master Nov 6, 2017
@bdunne bdunne assigned bdunne and unassigned NickLaMuro Nov 6, 2017
@bdunne bdunne added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 6, 2017
@Fryguy Fryguy deleted the expand_path_mime_types branch November 28, 2017 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants