move exception into Kubeclient namespace #195

grosser · 2016-10-01T22:25:32Z

seems weird that it got defined in the top-level namespace ...

simon3z · 2016-10-04T14:04:50Z

IIUC this is not backward-compatible. We may have to properly schedule this because we just released a major version. @moolitayer can you review?

grosser · 2016-10-04T16:24:57Z

... made it backwards compatible by defining the constant ...

moolitayer · 2016-10-05T11:41:35Z

Generally agree. Should not merge until we mean to version bump.

moolitayer · 2016-10-05T11:46:11Z

This would need a line in Readme in the Upgrading section

grosser · 2016-10-05T16:05:18Z

it is backwards compatible now ... so can merge ? ... also added a line to the upgrading section

moolitayer · 2016-10-05T16:58:00Z

This does not appear to be backward compatible, you are changing tests code to handle the new exception class. Please squash the change and have it ready to merge once we agree on a bump.

I will create an issue to track all of our non backward compatible changes

grosser · 2016-10-05T17:07:53Z

It is backwards compatible because the old constant still exists.
I updated the tests to use the new constant so we are not using a deprecated feature.

+class KubeException < Kubeclient::HttpException

grosser · 2016-10-05T17:17:12Z

build has same errors as on master ... added a minor cleanup to avoid defining a global method

grosser · 2016-10-11T17:01:16Z

@moolitayer ok to merge ?

grosser · 2016-10-27T15:22:00Z

@moolitayer ok to merge or what do you want changed ?

moolitayer · 2016-10-30T08:44:23Z

test/test_config.rb

+
+  def config_file(name)
+    File.new(File.join(File.dirname(__FILE__), 'config', name))
+  end


what is this change for (test_config_file => config_file)?

please add a test for the backward compatibility you are maintaining. Then I'm good with this change. @simon3z please review, are we going to merge anything before the integration problem is fixed or should that be fixed first?

things that start with test_ are tests, so I found this confusing/dangerous ... so changing to config_file

... and it was a global method ... moving it inside the test where it belongs caused it to run as test

👍 for this change, but things usually have a better change of being review if they are as restricted as possible...

grosser · 2016-10-30T17:30:00Z

test added!

moolitayer · 2016-10-31T07:56:52Z

LGTM @simon3z please review

simon3z · 2016-11-02T17:32:02Z

lib/kubeclient/http_exception.rb

@@ -0,0 +1,20 @@
+# TODO: remove this on next major version bump
+class KubeException < StandardError


@grosser don't you need the body here? (initialize, etc.)

Technically I suppose it works because you throw a Kubeclient::HttpException which contains the relevant attributes. Although to be strict, just by how this is defined, there could be KubeException with no :error_code, :message, :response, which is not backward compatible.

grosser · 2016-11-07T18:25:57Z

ready!

moolitayer · 2017-03-14T07:21:20Z

test/test_kubeclient.rb

+    error_message = 'certificate verify failed'
+
+    stub_request(:get, 'http://localhost:8080/api')
+      .to_raise(OpenSSL::SSL::SSLError.new(error_message))


So this exception is caught by rest-client and translated to a rest client error(since we only catch those afaik)?
Can you make it more straight forward (like the test above?)

see above please

I added this test to say "if an ssl error happens, kubeclient should react this way" ... I it wanted it to not matter what plumbing layer is in-between.

moolitayer · 2017-03-14T07:23:07Z

@simon3z is this direction desired? (I think this is generally good for us)

simon3z · 2017-03-14T07:33:21Z

I am OK with this change in general. @cben please review.
@grosser can you rebase?

cben · 2017-04-18T10:33:09Z

@grosser could you fix the conflicts?

grosser · 2017-04-18T15:27:06Z

updated

simon3z · 2017-04-18T15:31:30Z

@cben con you review the (minor) change/rebase and re-approve? I think we can merge this.

cben

thanks!

kirs · 2017-04-18T21:19:19Z

🎉

ManageIQ/kubeclient#195

cben · 2018-01-14T13:43:20Z

@moolitayer @grosser This is safe for next 2.x release we're assembling, but we need to fix README note (on master too) to use correct class name and speak of 3.0 not 2.0 (?)

grosser · 2018-01-15T04:13:43Z

so you are planing to remove KubeException in v3 ?
not really sure what needs to be done, can you whip up a PR or just change the docs ?

move exception into Kubeclient namespace # Conflicts: # README.md # lib/kubeclient/common.rb

Reverts parts of commit 263ff40 from ManageIQ#271 that restored KubeException - no longer needed after backporting ManageIQ#195 and ManageIQ#233, we now have Kubeclient::HttpError and Kubeclient::ResourceNotFoundError. In other words, v2.x branch is now closer to original ManageIQ#262.

This reverts commit 263ff40 from ManageIQ#271. Those backport changes are no longer needed: - after backporting ManageIQ#195 and ManageIQ#233, v2.x now has Kubeclient::HttpError and Kubeclient::ResourceNotFoundError. - after backporting ManageIQ#247, v2.x now tests with webmock 2.x, it takes `basic_auth` rather than user:pw in URL. - ns/namespace change was just cosmetic. In other words, v2.x branch is now closer to original ManageIQ#262.

move exception into Kubeclient namespace # Conflicts: # README.md # lib/kubeclient/common.rb

This reverts commit 263ff40 from ManageIQ#271. Those backport changes are no longer needed: - after backporting ManageIQ#195 and ManageIQ#233, v2.x now has Kubeclient::HttpError and Kubeclient::ResourceNotFoundError. - after backporting ManageIQ#247, v2.x now tests with webmock 2.x, it takes `basic_auth` rather than user:pw in URL. - ns/namespace change was just cosmetic. In other words, v2.x branch is now closer to original ManageIQ#262.

moolitayer · 2018-01-30T15:40:18Z

@grosser #293

grosser force-pushed the grosser/exception branch 3 times, most recently from e1841fd to f9787c4 Compare October 3, 2016 17:58

grosser force-pushed the grosser/exception branch 2 times, most recently from 4ed385a to e8481bd Compare October 4, 2016 16:40

grosser force-pushed the grosser/exception branch from af07786 to 115c309 Compare October 5, 2016 17:07

grosser force-pushed the grosser/exception branch from 115c309 to f63d511 Compare October 5, 2016 17:16

moolitayer reviewed Oct 30, 2016

View reviewed changes

grosser force-pushed the grosser/exception branch from f63d511 to 5092d3e Compare October 30, 2016 17:28

simon3z reviewed Nov 2, 2016

View reviewed changes

grosser force-pushed the grosser/exception branch 2 times, most recently from fa5ad97 to 1b54fc2 Compare November 7, 2016 18:25

grosser mentioned this pull request Nov 7, 2016

Kubeclient next: tracker of non backward compatible issues #199

Closed

moolitayer mentioned this pull request Mar 14, 2017

Introduce Kubeclient::ResourceNotFoundError #233

Merged

moolitayer reviewed Mar 14, 2017

View reviewed changes

cben approved these changes Mar 25, 2017

View reviewed changes

moolitayer approved these changes Mar 26, 2017

View reviewed changes

grosser mentioned this pull request Mar 27, 2017

bump rubocop to enforce what our style dictates #236

Merged

cben mentioned this pull request Mar 27, 2017

SSLError not always translated to KubeException #240

Open

grosser force-pushed the grosser/exception branch from 2985bdc to 96f9a82 Compare April 18, 2017 15:24

move exception into Kubeclient namespace

87b8ff1

grosser force-pushed the grosser/exception branch from 96f9a82 to 87b8ff1 Compare April 18, 2017 15:27

cben approved these changes Apr 18, 2017

View reviewed changes

simon3z merged commit 745a510 into ManageIQ:master Apr 18, 2017

grosser added a commit to zendesk/samson that referenced this pull request Nov 21, 2017

remove deprecated KubeException

785d382

ManageIQ/kubeclient#195

grosser added a commit to zendesk/samson that referenced this pull request Nov 21, 2017

remove deprecated KubeException

03021b8

ManageIQ/kubeclient#195

cben added the v2.x/yes label Jan 14, 2018

grosser deleted the grosser/exception branch January 15, 2018 04:08

grosser restored the grosser/exception branch January 15, 2018 04:10

cben added a commit to cben/kubeclient that referenced this pull request Jan 22, 2018

Merge pull request ManageIQ#195 from grosser/grosser/exception

5d3e5df

move exception into Kubeclient namespace # Conflicts: # README.md # lib/kubeclient/common.rb

cben mentioned this pull request Jan 22, 2018

[v2.x] backports for 2.6 release #289

Closed

moolitayer mentioned this pull request Jan 23, 2018

[WIP] Remove deprecated KubeException #293

Closed

cben added a commit to cben/kubeclient that referenced this pull request Jan 25, 2018

Merge pull request ManageIQ#195 from grosser/grosser/exception

d46d052

move exception into Kubeclient namespace # Conflicts: # README.md # lib/kubeclient/common.rb

grosser deleted the grosser/exception branch January 30, 2018 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move exception into Kubeclient namespace #195

move exception into Kubeclient namespace #195

grosser commented Oct 1, 2016

simon3z commented Oct 4, 2016

grosser commented Oct 4, 2016

moolitayer commented Oct 5, 2016

moolitayer commented Oct 5, 2016

grosser commented Oct 5, 2016

moolitayer commented Oct 5, 2016

grosser commented Oct 5, 2016

grosser commented Oct 5, 2016

grosser commented Oct 11, 2016

grosser commented Oct 27, 2016

moolitayer Oct 30, 2016

moolitayer Oct 30, 2016 •

edited

Loading

grosser Oct 30, 2016

grosser Oct 30, 2016

moolitayer Mar 14, 2017 •

edited

Loading

grosser commented Oct 30, 2016

moolitayer commented Oct 31, 2016

simon3z Nov 2, 2016

grosser Nov 4, 2016

grosser commented Nov 7, 2016

moolitayer Mar 14, 2017

moolitayer Mar 14, 2017

grosser Mar 15, 2017

moolitayer commented Mar 14, 2017 •

edited

Loading

simon3z commented Mar 14, 2017

cben commented Apr 18, 2017

grosser commented Apr 18, 2017

simon3z commented Apr 18, 2017

cben left a comment

kirs commented Apr 18, 2017

cben commented Jan 14, 2018

grosser commented Jan 15, 2018

moolitayer commented Jan 30, 2018

		@@ -0,0 +1,20 @@
		# TODO: remove this on next major version bump
		class KubeException < StandardError

move exception into Kubeclient namespace #195

move exception into Kubeclient namespace #195

Conversation

grosser commented Oct 1, 2016

simon3z commented Oct 4, 2016

grosser commented Oct 4, 2016

moolitayer commented Oct 5, 2016

moolitayer commented Oct 5, 2016

grosser commented Oct 5, 2016

moolitayer commented Oct 5, 2016

grosser commented Oct 5, 2016

grosser commented Oct 5, 2016

grosser commented Oct 11, 2016

grosser commented Oct 27, 2016

Choose a reason for hiding this comment

moolitayer Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

grosser commented Oct 30, 2016

moolitayer commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Mar 14, 2017 • edited Loading

simon3z commented Mar 14, 2017

cben commented Apr 18, 2017

grosser commented Apr 18, 2017

simon3z commented Apr 18, 2017

cben left a comment

Choose a reason for hiding this comment

kirs commented Apr 18, 2017

cben commented Jan 14, 2018

grosser commented Jan 15, 2018

moolitayer commented Jan 30, 2018

moolitayer Oct 30, 2016 •

edited

Loading

moolitayer Mar 14, 2017 •

edited

Loading

moolitayer commented Mar 14, 2017 •

edited

Loading