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

moving settings from manageiq #91

Merged
merged 2 commits into from
Jan 6, 2017
Merged

moving settings from manageiq #91

merged 2 commits into from
Jan 6, 2017

Conversation

durandom
Copy link
Member

@durandom durandom commented Dec 13, 2016

I will remove the settings from manageiq/config/settings.yml once this is merged

# :spec-region-1:
# :name: Special Region
# :hostname: ec2.spec-region-1.amazonaws.com
# :description: Super Special Region
Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare I added this as dummy example

Copy link
Member

Choose a reason for hiding this comment

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

@durandom Nice!

@durandom
Copy link
Member Author

@blomquisg care to merge this?

:ems_amazon:
:disabled_regions: []
# disable regions by their keys found in app/models/manageiq/providers/amazon/regions.rb e.g.
# uncomment the following to disable gov-west-1 region
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this statement. We don't want users editing this file at all. Instead they would go either go through Advanced Settings (which stores the changes in the database) or we would have them deploy a settings.local.yml or something.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this comment will not appear in Advanced Settings, so who is the audience for this comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right about the uncomment statement.

Note that this comment will not appear in Advanced Settings

Yeah, I know, but I havent found a better place to add some configuration hints. Same as here

I think it's better than nothing. And advanced users might find their way by googleing or looking at the source. And if its only support staff of ours.

Would it be better to approach the docs team to add that to the docs?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was just thinking docs. That seems the most appropriate place to document new settings. You may be able to contribute directly in https://github.com/ManageIQ/manageiq_docs, particularly if they already have section somewhere for Advanced Settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will open a PR to that repo 👍

@miq-bot
Copy link
Member

miq-bot commented Jan 6, 2017

Checked commits durandom/manageiq-providers-amazon@bf6b305~...d2b3710 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 🏆

@Fryguy Fryguy merged commit ef38dc1 into ManageIQ:master Jan 6, 2017
@durandom durandom deleted the settings branch January 8, 2017 18:15
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