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

Operation callbacks fix #207

Closed

Conversation

israel-hdez
Copy link
Member

Created branch 2.8.x on the repo from v2.8.0 tag. On top of that branch/tag, cherry-picked commit 49fc84a. I will need this released as v2.8.1.

This is required to fix BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1443005.

Note 1: v2.9.0 included that commit/fix, so there is no problem in that version.
Note 2: Travis is raising errors when testing on live servers. I don't know why that's happening 😞. In travis log, something is echoed saying that hawkular-services was the one that failed to perform an operation. So, may be the problem is somewhere else?

jkremser and others added 2 commits April 20, 2017 17:23
If the operation is called with the operation name given by
symbol, instead of the string. It's performed correctly on the agent,
but the failure callback is called.
For ruby 2.2 travis uses a version of bundler that is not resolving
dependencies correctly. Adding command to update bundler's version to
make the build not fail due to dependencies wrongly resolved.
@josejulio
Copy link
Member

josejulio commented Apr 21, 2017

Do we really need to fix that with v2.8.1? Can't they use v2.9 ?

@coveralls
Copy link

coveralls commented Apr 21, 2017

Coverage Status

Coverage remained the same at 96.042% when pulling 07d713b on israel-hdez:operation_callbacks_fix into 98e62aa on hawkular:2.8.x.

@josejulio
Copy link
Member

Note 2: Travis is raising errors when testing on live servers. I don't know why that's happening . In travis log, something is echoed saying that hawkular-services was the one that failed to perform an operation. So, may be the problem is somewhere else?

Maybe the server (hawkular-services) is too new for v2.8. If we are going to support old versions we should consider on specifying a hawkular-services version to test against instead of using the latest version.
see docker-compose

@israel-hdez
Copy link
Member Author

israel-hdez commented Apr 21, 2017

@josejulio Well... I would say yes. But by doing that, some VCR cassettes in MiQ will need to be re-recorded. I want to avoid that since that BZ is happening in CFME 5.8.0, which is based in MiQ "fine", which is only receiving bug-fixes.
I will check if testing against an older server version fixes the tests.

@pilhuhn
Copy link
Member

pilhuhn commented Apr 21, 2017

@israel-hdez " is echoed saying that hawkular-services was the one that failed to perform an operation." I think the agent inside h-services is set to immutable (as we are running in-container in Travis) and thus operations can indeed not be peformed. I think @josejulio had a fix for this some time ago?

@jkremser
Copy link
Member

Supporting multiple minor versions and back-porting stuff may complicate the whole process, but sooner or later we have to probably do that anyway.

@josejulio perhaps it make sense to run the live tests only for the master branch and PRs against master (?)

lgtm, I'd merge it despite the travis red, if anyone doesn't object

@josejulio
Copy link
Member

What versions of hawkular-services are we going to support with hawkular-ruby-client v2.8? If we are going to support latest version, then our live tests should pass on that version, else we won't be sure if all the tests run.

Live testing was working since v.2.8. If we aren't going to support latest (hawkular-services) version we should run the tests with the latest hawkular-services supported version by hawkular-client-ruby v2.8.

Having tests running on old cassettes (about 6 months ago) is almost as having no tests, but that's just my opinion :)

@josejulio
Copy link
Member

josejulio commented Apr 21, 2017

@josejulio Well... I would say yes. But by doing that, some VCR cassettes in MiQ will need to be re-recorded. I want to avoid that since that BZ is happening in CFME 5.8.0, which is based in MiQ "fine", which is only receiving bug-fixes.

I'm not sure what would be the problem on recording the tests again, we might need to support less stuff this way.

@israel-hdez
Copy link
Member Author

I'm not sure what would be the problem on recording the tests again, we might need to support less stuff this way.

@josejulio You resist to bring support, haha. But I understand you ;) I would have to sync with other people also using the gem, so that they also re-record their cassettes. I just want to avoid that hassle.

Anyway, tonight my pillow suggested me a way to fix the BZ in MiQ/fine side. I can apply that fix and we can forget about this PR. However, MiQ/fine will still be a "user" of v2.8.0 and that forces us to bring support to that version if something else arises.

@josejulio
Copy link
Member

@josejulio You resist to bring support, haha. But I understand you ;) I would have to sync with other people also using the gem, so that they also re-record their cassettes. I just want to avoid that hassle.

I understand it can be a hassle. But think about it, If 2.8 is failing on live tests, its a hint that something may be wrong.
I'm just suggesting that if we aren't aiming to support latest hawkular-services with v2.8, we just set the right version on the docker-compose, so our tests will run fine (or fix the tests with #186 and maybe #191).
although I'm more in favor of bumping the version to 2.9, it should be backwards compatible since we are trying to use semantic versioning.

@israel-hdez
Copy link
Member Author

@josejulio a short discussion about bumping the version happened in ManageIQ/manageiq-gems-pending#102. Even with semantic versioning the suggestion was to lock to a specific version, and that's the reason why I am keeping fine in 2.8.x.

Well.... Too much discussion about support/not-support previous versions. So, I placed ManageIQ/manageiq#14851 to fix the BZ in MiQ side.

We should agree on how to handle these kind of issues. If we are going to support previous versions, I can fix the specs as you @josejulio suggested in your last comment to have a green on 2.8.x branch.

@josejulio
Copy link
Member

We should agree on how to handle these kind of issues. If we are going to support previous versions, I can fix the specs as you @josejulio suggested in your last comment to have a green on 2.8.x branch.

That would be awesome, thanks!

@israel-hdez
Copy link
Member Author

I'll close this. It doesn't apply anymore, because MiQ fine was updated to use 3.0.1.

@israel-hdez israel-hdez deleted the operation_callbacks_fix branch May 2, 2017 15:26
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.

5 participants