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

Remove "ovirt-engine" prefix from paths #55

Merged
merged 1 commit into from
Jul 7, 2016
Merged

Remove "ovirt-engine" prefix from paths #55

merged 1 commit into from
Jul 7, 2016

Conversation

jhernand
Copy link
Contributor

@jhernand jhernand commented Jun 27, 2016

The path passed to the "api_uri" method will have the "/api" prefix if it is
extracted from the "ems_ref" attribute stored in the database, and will have
the "/ovirt-engine/api" if it comes directly from the "href" attribute of the
XML documents, for example when using the "relationships" method to fetch
secondary objects related to the primary object. This means that to have a
clean path we need to remove both "ovirt-engine" and "api".

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

@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage increased (+0.7%) to 70.075% when pulling 9d597a5 on jhernand:remove_ovirt_engine_api_from_passed_path into d092737 on ManageIQ:master.

@jhernand
Copy link
Contributor Author

@bdunne, @blomquisg, @pkliczewski, please review.

This is needed in order to fix the integration with version 4 of the engine when using targeted refresh. Note that we also need a change in the provider in order to calculate correctly the ems_ref attribute, removing the /ovirt-engine/ prefix. I will shortly submit that PR.

Once this is merged we will need a new release of the gem.

@jhernand
Copy link
Contributor Author

The travis build for Ruby 1.9.3 failed with the following error:

An error occurred while installing mime-types (3.1), and Bundler cannot
continue.

I think it isn't related to this PR.

API_PATH = "/ovirt-engine/api"

it "removes slash" do
allow(service).to receive(:base_uri).and_return(BASE_URI)
Copy link
Member

Choose a reason for hiding this comment

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

You can share similar test setup in a before block in this context. I'd prefer the allow to be an expect since we know that it will be called and should only be called once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice Brandon, I addressed it in the new version of the PR.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage increased (+0.6%) to 70.014% when pulling 5b5edfd on jhernand:remove_ovirt_engine_api_from_passed_path into d092737 on ManageIQ:master.

@jhernand
Copy link
Contributor Author

@bdunne, @blomquisg, the Travis build is failing for Ruby 1.9.3 because it detects that one of the dependencies (mime-types) requires Ruby version >= 2.0. Does it make sense to keep testing this gem with Ruby 1.9.3 when ManageIQ itself already requires Ruby 2.2?

@bdunne
Copy link
Member

bdunne commented Jun 29, 2016

@jhernand I'm dropping support for 1.9.3 here: #56

@jhernand
Copy link
Contributor Author

Thanks @bdunne, I will rebase once that is merged.

@bdunne
Copy link
Member

bdunne commented Jun 30, 2016

@jhernand #56 is merged, please rebase

@jhernand
Copy link
Contributor Author

jhernand commented Jul 1, 2016

@bdunne, rebased.

@jhernand
Copy link
Contributor Author

jhernand commented Jul 1, 2016

@bdunne the Travis build is failing now because coincidentally activesupport 5.0.0 was released yesterday, and it requires Ruby >= 2.2.2. As our requirement doesn't specify a version bound Travis is getting the latest one.

I see that ManageIQ itself requires 5.0.x, from git, it should probably be updated to require the gem, now that it is released.

I'd suggest that we exclude all Ruby versions older than 2.2.2 from the Travis builds.

@jhernand
Copy link
Contributor Author

jhernand commented Jul 1, 2016

I have added to this PR a commit to remove from the Travis configuration the versions of Ruby older than 2.2.2. That should probably be part of a different PR, but I just want to check that everything works correctly. If required, I will move that patch to a different PR, later.

@jhernand
Copy link
Contributor Author

jhernand commented Jul 1, 2016

@bdunne, the build works correctly with Ruby 2.2.2 and Ruby 2.3.1. Do you want a separate PR to remove older versions? Or should we address this issue in a different way?

@jhernand
Copy link
Contributor Author

jhernand commented Jul 5, 2016

@bdunne, any chance we can merge this and do a new release of the gem before we decide how to better resolve the issue with Rails 5 and activesupport?

The path passed to the "api_uril" method will have the "/api" prefix if it is
extracted from the "ems_ref" attribute stored in the database, and will have
the "/ovirt-engine/api" if it comes directly from the "href" attribute of the
XML documents, for example when using the "relationships" method to fetch
secondary objects related to the primary object. This means that to have a
clean path we need to remove both "ovirt-engine" and "api".

Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
@jhernand
Copy link
Contributor Author

jhernand commented Jul 7, 2016

Removed the patch that changes the Travis configuration, only the original patch remains.

@jhernand
Copy link
Contributor Author

jhernand commented Jul 7, 2016

All the tests passed, @bdunne, @Fryguy can we merge this and make a new release of the gem, in order to then update ManageIQ to use the new version?

@bdunne bdunne merged commit 9b4cf08 into ManageIQ:master Jul 7, 2016
bdunne added a commit that referenced this pull request Jul 7, 2016
- Breaking changes
  - Drop support for Ruby 1.9.3 [#56]

- Other changes
  - Remove "ovirt-engine" prefix from paths [#55]
  - Drop dependency on ActiveSupport due to Rails 5 minimum Ruby version [#57]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants