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 internal attribute to service template #17781

Merged

Conversation

hsong-rh
Copy link
Contributor

@hsong-rh hsong-rh commented Jul 31, 2018

Add internal attribute to service template table.

Depend on PR: ManageIQ/manageiq-schema#238

https://bugzilla.redhat.com/show_bug.cgi?id=1594408

@hsong-rh hsong-rh force-pushed the add_internal_attribut_for_upstream branch from 194b3de to 34da8f1 Compare July 31, 2018 18:35
@hsong-rh
Copy link
Contributor Author

@bzwei @gmcculloug Please review.

@gmcculloug gmcculloug self-assigned this Jul 31, 2018
@gmcculloug gmcculloug requested a review from bzwei July 31, 2018 18:45
@Fryguy
Copy link
Member

Fryguy commented Aug 2, 2018

@hsong-rh Since this is upstream, can you remove the upstream wording from your commit and PR title as they make no sense.

@hsong-rh hsong-rh changed the title Add internal attribute to service template (commits for upstream only) Add internal attribute to service template Aug 2, 2018
@hsong-rh hsong-rh force-pushed the add_internal_attribut_for_upstream branch from 34da8f1 to 28ee034 Compare August 2, 2018 19:16
@hsong-rh
Copy link
Contributor Author

hsong-rh commented Aug 2, 2018

@Fryguy Words removed.

@miq-bot
Copy link
Member

miq-bot commented Aug 2, 2018

Checked commit hsong-rh@28ee034 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. ⭐

@hsong-rh hsong-rh closed this Aug 9, 2018
@hsong-rh hsong-rh reopened this Aug 9, 2018
@hsong-rh
Copy link
Contributor Author

hsong-rh commented Aug 9, 2018

@gmcculloug Schema PR was merged, build passed. Please merge. Thanks.

@gmcculloug gmcculloug merged commit adf07b4 into ManageIQ:master Aug 9, 2018
@gmcculloug gmcculloug added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 9, 2018
@hsong-rh hsong-rh deleted the add_internal_attribut_for_upstream branch August 9, 2018 17:43
@lpichler
Copy link
Contributor

lpichler commented Sep 18, 2018

@hsong-rh @gmcculloug @bzwei I think I found a bug.

which boolean value means ServiceTemplate#internal = nil ?

objects.second.class =>
ServiceTemplateCatalog

(byebug) objects.second.service_templates.map {|x| [x.service_template_catalog_id, x.internal] } =>
[[75000000000001, nil], [75000000000001, nil], [75000000000001, nil], [75000000000001, nil]]

objects.second.service_templates.public_service_templates =>
[]

objects.second.service_templates.where.not(:internal => true) =>
[]

it is not considering NULL values because of so-called three value logic.

https://stackoverflow.com/questions/17679721/why-does-postgresql-not-return-null-values-when-the-condition-is-true

or NULL values should be covered by sql migration ?

thanks!

@hsong-rh
Copy link
Contributor Author

@lpichler We shouldn't have the case of internal = nil. In the base class, its default value is set to false, and only be true if the type is ServiceTemplateTransformationPlan. Are objects.second.service_templates the type of ServiceTemplateTransformationPlan?

@lpichler
Copy link
Contributor

lpichler commented Sep 18, 2018

@hsong-rh No, type is nil. And it is possible because of internal column has been added
https://github.com/ManageIQ/manageiq-schema/pull/238/files#diff-1d061c844b9d792d54472130a429e360R10 without default value false - which is ok according to our recommendations.

but in this case you have to set false value in migration to cover also other ServiceTemplate or change the query.

@bzwei
Copy link
Contributor

bzwei commented Sep 18, 2018

@lpichler I were aware of this three value logic issue when I was testing this PR. I could not remember what was the code that passed all manual tests nor can I understand how the PR finally ended up at this state. Nevertheless the following fix seems working:

scope :public_service_templates,                  ->         { where(:internal => [false, nil]) }

@gmcculloug
Copy link
Member

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.

6 participants