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.

Sign up for GitHub

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

Jump to bottom

Add archive/unarchive actions to ServiceTemplate #389

Merged
merged 1 commit into from
Jun 5, 2018
Merged

Add archive/unarchive actions to ServiceTemplate #389

merged 1 commit into from
Jun 5, 2018

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 1, 2018 •
edited by abellotti
Loading

Add the ability to archive and unarchive service templates

Core PRs: ManageIQ/manageiq-schema#207, ManageIQ/manageiq#17480

ManageIQ/manageiq-v2v#314

@agrare agrare force-pushed the archive_unarchive_service_template branch from 7402c0c to 41617d0 Compare June 1, 2018 19:19
@kbrock
Copy link
Member

kbrock commented Jun 1, 2018

Think this failure is on me
The first commit for #385 may fix this

@agrare agrare force-pushed the archive_unarchive_service_template branch from 41617d0 to 7a70929 Compare June 4, 2018 13:35
@agrare
Copy link
Member Author

agrare commented Jun 4, 2018

Thanks @kbrock that fixed two of the failures but still getting 10 other errors on master+your commit 😟

@agrare
Copy link
Member Author

agrare commented Jun 4, 2018

@abellotti added specs, please take a look

@miq-bot assign @abellotti

@agrare agrare mentioned this pull request Jun 4, 2018
Closed
@agrare agrare force-pushed the archive_unarchive_service_template branch from 7a70929 to 6db5d11 Compare June 4, 2018 15:40
@abellotti abellotti added this to the Sprint 87 Ending Jun 4, 2018 milestone Jun 4, 2018
abellotti
abellotti reviewed Jun 4, 2018
View reviewed changes
app/controllers/api/service_templates_controller.rb Outdated
service_template.unarchive!
action_result(true, "Activated Service Template")
rescue => err
action_result(false, "Could not activate Service Template - #{err}")
Copy link
Member

Choose a reason for hiding this comment

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

maybe Could not unarchive ... ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

spec/requests/service_templates_spec.rb
expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(expected)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

we also need another set of the above tests for unarchive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@agrare agrare force-pushed the archive_unarchive_service_template branch from 6db5d11 to b332cf7 Compare June 4, 2018 17:16
@agrare agrare mentioned this pull request Jun 4, 2018
Merged
@agrare
Copy link
Member Author

agrare commented Jun 4, 2018

Hey @abellotti a few spec failures that I don't understand:

https://travis-ci.org/ManageIQ/manageiq-api/jobs/387898964#L1946-L1949

  1. API configuration (config/api.yml) collections identifiers contains only valid miq_feature identifiers
    Failure/Error: expect(dangling).to be_empty
    expected #<Set: {"svc_catalog_archive", "svc_catalog_unarchive"}>.empty? to return true, got false

This one I get and ManageIQ/manageiq#17518 fixes it

I don't understand these two:
https://travis-ci.org/ManageIQ/manageiq-api/jobs/387898964#L1951-L1959

  1. Service Templates API Service Templates order with an unorderable template does not show the order action
    Failure/Error: expect(actions).to_not include("order")
    expected ["edit", "edit", "edit", "order"] not to include "order"
    # ./spec/requests/service_templates_spec.rb:547:in `block (4 levels) in <top (required)>'
  2. Service Templates API Service Templates unarchive shows the action
    Failure/Error: expect(actions).to include("unarchive")
    expected ["order"] to include "unarchive"
    # ./spec/requests/service_templates_spec.rb:650:in `block (3 levels) in <top (required)>'

@michaelkro michaelkro mentioned this pull request Jun 5, 2018
Merged
abellotti
abellotti reviewed Jun 5, 2018
View reviewed changes
config/api.yml Outdated
- :name: archive
:identifier: svc_catalog_archive
- :name: unarchive
:identifier: svc_catalog_unarchive
:options:
- :validate_action
Copy link
Member

Choose a reason for hiding this comment

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

reason for the one failure, options: validate_action here is meant for the order action on 2805, so move archive/unarchive below 2812.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 thanks

config/api.yml
- :name: archive
:identifier: svc_catalog_archive
- :name: unarchive
:identifier: svc_catalog_unarchive
Copy link
Member

Choose a reason for hiding this comment

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

the other failures might be because svc_catalog_archive and svc_catalog_unarchive are not mid_product_feature.yml

Copy link
Member Author

Choose a reason for hiding this comment

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

Think this was actually related to #389 (comment) since I had ManageIQ/manageiq#17518 applied locally when testing and this still failed.

@agrare agrare force-pushed the archive_unarchive_service_template branch from b332cf7 to 7208406 Compare June 5, 2018 12:48
@agrare
Copy link
Member Author

agrare commented Jun 5, 2018

@abellotti thanks! with ManageIQ/manageiq#17518 applied locally tests pass. Can you take a look at that one.

@miq-bot
Copy link
Member

miq-bot commented Jun 5, 2018

Checked commit agrare@7208406 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 2 offenses detected

app/controllers/api/service_templates_controller.rb

@agrare agrare closed this Jun 5, 2018
@agrare agrare reopened this Jun 5, 2018
@abellotti
Copy link
Member

This LGTM!! Thanks @agrare for the API Enhancement. 👍

@abellotti abellotti merged commit 97dd729 into ManageIQ:master Jun 5, 2018
@agrare agrare deleted the archive_unarchive_service_template branch June 5, 2018 19:57
@agrare
Copy link
Member Author

agrare commented Jun 5, 2018

❤️ thanks @abellotti

@abellotti abellotti changed the title Add archive/activate actions to ServiceTemplate Add archive/unarchive actions to ServiceTemplate Jun 5, 2018
@abellotti abellotti mentioned this pull request Jun 5, 2018
Merged
@AparnaKarve
Copy link
Contributor

@miq-bot add_label gaprindashvili/yes

@AparnaKarve
Copy link
Contributor

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

@miq-bot add_label blocker

simaishi pushed a commit that referenced this pull request Jun 21, 2018
@abellotti @simaishi
Merge pull request #389 from agrare/archive_unarchive_service_template
adf6197 
Add archive/activate actions to ServiceTemplate
(cherry picked from commit 97dd729)

https://bugzilla.redhat.com/show_bug.cgi?id=1594023
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit adf6197903a80d3275b6a5eeb2f920509a0380e9
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Jun 5 15:57:48 2018 -0400

    Merge pull request #389 from agrare/archive_unarchive_service_template
    
    Add archive/activate actions to ServiceTemplate
    (cherry picked from commit 97dd7298aea755a7a387f554d02c2e5706f408b2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abellotti abellotti abellotti approved these changes

Assignees

@abellotti abellotti

Labels
blocker enhancement gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@agrare @kbrock @miq-bot @abellotti @AparnaKarve @simaishi