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

Add state to services #356

Merged
merged 1 commit into from
Mar 25, 2019
Merged

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Mar 25, 2019

Per https://bugzilla.redhat.com/show_bug.cgi?id=1677571 services need a state column that we can jack for provisioning n retirement and whatever else might change state of a service. Booya.

Actually I just wanted to see if anyone would read it, that's why there's a booya.

@miq-bot
Copy link
Member

miq-bot commented Mar 25, 2019

Checked commit d-m-u@d6e9034 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare self-assigned this Mar 25, 2019
@agrare
Copy link
Member

agrare commented Mar 25, 2019

@d-m-u there's a retirement_state column already, is this expected to be a more general replacement for that or will they compliment each other? Like state is "retired" and retirement_state is "in progress" ?

@d-m-u
Copy link
Contributor Author

d-m-u commented Mar 25, 2019

Yeah @agrare we wanted a more general one. I think Madhu said something about phasing out the retirement specific state eventually, or not, but in any case, a general place for state things was asked for.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Booya

@agrare agrare merged commit 2b4493d into ManageIQ:master Mar 25, 2019
@agrare agrare added this to the Sprint 108 Ending Apr 1, 2019 milestone Mar 25, 2019
@d-m-u d-m-u deleted the adding_state_to_service branch March 25, 2019 18:27
@carbonin
Copy link
Member

Is this something that really needs to be a column?
Based on this BZ comment from @tinaafitz it sounds like there would be a way to get this information without putting it in a new column:

there's no way to tell if it was provisioned successfully without looking at the request

That sounds like something that belongs more in a method than a net-new piece of data.

@carbonin
Copy link
Member

Shouldn't this just be Service#miq_request_task.state or something?

@carbonin
Copy link
Member

I feel like the purpose of this is to evaluate the current state of the whole service, but if the provisioning is still in progress wouldn't that need to be evaluated dynamically?

@tinaafitz
Copy link
Member

tinaafitz commented Mar 25, 2019

@carbonin Thanks for the feedback. Our goal is to eventually use the state column for provisioning
and retirement, phasing out retirement_state as @d-m-u and @agrare discussed here: #356 (comment)

@carbonin
Copy link
Member

Okay, that's fine for now I guess.

But, unless we're planning that refactoring immediately I would have preferred this have a more specific name like provisioning_state to differentiate it from retirement_state.

And additionally, when adding something like this to the schema, please try to offer some additional information about the intended design. There's no detail in the BZ about what is trying to be accomplished and the PR/commit also offers no details about why a column is being added. As it stands it's impossible for any developer other than @d-m-u and @tinaafitz to learn what the function of this column should be.

@Fryguy
Copy link
Member

Fryguy commented Mar 26, 2019

yanked from gitter:

@carbonin @agrare So, the automate team grabbed my ear this morning about #356 and it's use in general
I think the takeaway for me is that there are a number of cases where we are presenting buttons, such as retirable, at different lifecycle states
My proposal to the team was to have model methods (or perhaps use support_feature) that the UI can use (e.g. a retirable? method)
additionally, we need a lot more information about what issues they are trying to solve, so much more documentation.
I don't think we can choose a proper path unless we have that information first
oh, with the model methods, I also mirrored your suggestion to try and use existing data where possible so as not to introduced column synchronization
so, they are going to get that information, and then we can discuss this further and see if we should revert #356 or change it (I recommended if we keep it to rename it to something less general, perhaps lifecycle_state)

@tinaafitz
Copy link
Member

@Fryguy Thanks for the discussion and for adding detail here.
@carbonin Thanks for the suggestions. I understand the process better now.

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.

7 participants