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

fix cli set timezone doesn't work #21

Merged
merged 2 commits into from
Dec 6, 2017

Conversation

ailisp
Copy link
Member

@ailisp ailisp commented Dec 1, 2017

Issue:
Set timezone in cli doesn't work, because the new timezone parameter isn't sent to TimezoneConfiguration class. It's fixed now.
BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1517897

\cc @carbonin, @yrudman, @gtanzillo

@miq-bot add-label bug

@ailisp
Copy link
Member Author

ailisp commented Dec 1, 2017

@miq-bot add-label wip

@miq-bot miq-bot added the bug label Dec 1, 2017
@miq-bot miq-bot changed the title fix cli set timezone doesn't work [WIP] fix cli set timezone doesn't work Dec 1, 2017
@miq-bot miq-bot added the wip label Dec 1, 2017
@ailisp
Copy link
Member Author

ailisp commented Dec 1, 2017

@miq-bot remove-label wip

@miq-bot miq-bot changed the title [WIP] fix cli set timezone doesn't work fix cli set timezone doesn't work Dec 1, 2017
@miq-bot miq-bot removed the wip label Dec 1, 2017
@@ -112,6 +112,6 @@ def configure_date_time
logger.error("Failed to apply time configuration: #{e.message}")
false
end
end # class TimezoneConfiguration
end # class DateTimeConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Why change this in this PR?

@@ -8,8 +8,9 @@ class TimezoneConfiguration
attr_reader :current_timzone
attr_accessor :new_timezone

def initialize(region_timezone_string)
def initialize(region_timezone_string, region_new_timezone_string = "")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a parameter here, couldn't the cli just set the new_timzone using the attr_accessor?

Then in a follow up maybe we can remove the current timezone from the parameter here because it doesn't seem to be used.

@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2017

Checked commits ailisp/manageiq-appliance_console@36e14e2~...bf3f65c with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@carbonin carbonin self-assigned this Dec 6, 2017
@carbonin carbonin merged commit 7f42253 into ManageIQ:master Dec 6, 2017
@carbonin carbonin added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants