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

Fix generation of resource handlers for OpenShift #1758

Merged
merged 8 commits into from
Sep 17, 2019

Conversation

Vlatombe
Copy link
Contributor

Broken since 25da8ca

@centos-ci
Copy link

Can one of the admins verify this patch?

@rohanKanojia
Copy link
Member

ok to test

Copy link
Member

@rohanKanojia rohanKanojia left a comment

Choose a reason for hiding this comment

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

@Vlatombe : Thanks for your PR. Could you please add the following:

  • a small test validating the desired change
  • a line to changelog about the change.

@Vlatombe
Copy link
Contributor Author

@rohanKanojia done

@@ -0,0 +1,66 @@
package io.fabric8.openshift.client;

Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a license header here? This command would add it automatically:

mvn -N license:format

@Vlatombe
Copy link
Contributor Author

@rohanKanojia Done.

@rohanKanojia
Copy link
Member

retest this please

import io.fabric8.openshift.api.model.v4_0.OAuthClient;
import io.fabric8.openshift.api.model.v4_0.PolicyBinding;
import io.fabric8.openshift.api.model.v4_0.Project;
import io.fabric8.openshift.api.model.v4_0.RoleBinding;
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this? I don't see any changes done in model with respect to this change. Could you please elaborate intention of having separate model for 4.0 openshift?

Copy link
Member

@rohanKanojia rohanKanojia left a comment

Choose a reason for hiding this comment

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

@Vlatombe : I didn't see closely but build seems to be failing, Could you please fix it whenever you get time?

@Vlatombe
Copy link
Contributor Author

@rohanKanojia Fixed. I guess I got confused because these imports showed up in the classpath in IDEA but I missed that it was not available in openshift-client. Anyway the first test is fine.

@rohanKanojia
Copy link
Member

@Vlatombe : Could you please update CHANGELOG also? we released 4.5.2 last weekend.

@rohanKanojia
Copy link
Member

We are good to merge this then 😄

@Vlatombe
Copy link
Contributor Author

@rohanKanojia You should consider using Release Drafter for changelog, it's really good. Under the @jenkinsci organization we've been using it for a while

@rohanKanojia
Copy link
Member

@Vlatombe : oh, cool. @oscerd @iocanel Wdyt?

@iocanel
Copy link
Member

iocanel commented Sep 17, 2019

@Vlatombe It looks good to me.

The only thing I would check, is that the changes done in the velocity template are algined with the rest of the modules. Its really handy if these variables are named the same across all templates, or we are going to have regressions next time we copy/paste something between them.

@fusesource-ci fusesource-ci merged commit ea514ee into fabric8io:master Sep 17, 2019
@Vlatombe
Copy link
Contributor Author

Vlatombe commented Sep 17, 2019

@iocanel yes, I aligned the variable names with the corresponding kubernetes templates.

@Vlatombe Vlatombe deleted the fix-openshift-handlers branch September 17, 2019 13:03
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

6 participants