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

bin/setup - create manageiq/log manually #273

Closed
wants to merge 2 commits into from
Closed

Conversation

cben
Copy link
Contributor

@cben cben commented Aug 21, 2018

Similar to ManageIQ/manageiq-ui-classic#4502, ManageIQ/manageiq-providers-nuage#130.

Since ManageIQ/manageiq#17663, there is no manageiq/log directory by default.
This breaks Travis e.g. https://travis-ci.org/ManageIQ/manageiq-providers-kubernetes/jobs/418623069

@miq-bot add-label bug, developer, test

@kbrock @himdel @miha-plesko please review

Copy link

@miha-plesko miha-plesko left a comment

Choose a reason for hiding this comment

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

LGTM. Would prefer native ruby solution like here ManageIQ/manageiq-providers-nuage#130 but it's up to you.

@cben
Copy link
Contributor Author

cben commented Aug 21, 2018

Switched to native ruby, PTAL
Deviated from ManageIQ/manageiq-providers-nuage#130 — kept it outside the if, as I think if you manually create or symlink a fresh spec/manageiq with log/, bin/setup should create it.

@miq-bot
Copy link
Member

miq-bot commented Aug 21, 2018

Checked commits cben/manageiq-providers-kubernetes@a41d214~...01bc248 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link

@miha-plesko miha-plesko left a comment

Choose a reason for hiding this comment

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

Good point @cben I like your solution because it (a) uses absolute path and will work regardless where it's called from and (b) what you say with IF, we should perform the call always. Will copy this one from you, thanks.

@cben
Copy link
Contributor Author

cben commented Aug 21, 2018

@kbrock @agrare can you merge (dont want to merge my own)

EDIT: hold on for gitter discussion to settle

@tadeboro
Copy link

I think that in case spec/manageiq is a symlink, bin/setup should not mess with developer managed content. When developer creates this symlink, he took the responsibility of keeping the things up and running.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Thanks so much for your feedback @cben

I'm hoping this fix in one place will fix the problem

ManageIQ/manageiq#17886

@cben cben closed this Aug 21, 2018
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.

5 participants