Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-9815 move CertService to more generic location - #2071
Merged
Conversation
this can be used in ApplicationClusters as well as in planned CA-plugins
DaanHoogland
commented
Apr 27, 2017
ContributorAuthor
@blueorangutan package |
blueorangutan
commented
Apr 27, 2017
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
DaanHoogland
commented
Apr 27, 2017
ContributorAuthor
@rhtyd @rafaelweingartner@wido please have a look, |
yadvr
commented
Apr 27, 2017
Member
LGTM. |
blueorangutan
commented
Apr 27, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-686 |
resmo
reviewed
Apr 27, 2017
| // under the License. | ||
| package com.cloud.network.lb; | ||
| package com.cloud.network.ssl; | ||
Member
There was a problem hiding this comment.
just a note, SSL is outdated, the official name would be TLS, name the package tls? https://en.wikipedia.org/wiki/Transport_Layer_Security
DaanHoogland
commented
Apr 27, 2017
ContributorAuthor
@blueorangutan test |
blueorangutan
commented
Apr 27, 2017
@DaanHoogland unsupported parameters provided. Supported mgmt server os are: |
DaanHoogland
commented
Apr 27, 2017
ContributorAuthor
@blueorangutan package |
blueorangutan
commented
Apr 27, 2017
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Apr 27, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-687 |
DaanHoogland
commented
Apr 27, 2017
ContributorAuthor
@blueorangutan test |
blueorangutan
commented
Apr 27, 2017
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
rafaelweingartner
commented
Apr 27, 2017
Member
@DaanHoogland as long as we are moving things aroung. |
blueorangutan
commented
Apr 27, 2017
Trillian test result (tid-1047)
|
ContributorAuthor
@rafaelweingartner I don't like to. Is this 👎 worth? Can of worms involved. There is a lot more service definitions to take into account when we do that move. We can but it is to much change for the scope of this one and only if we move all will it serve it purpose. Also this as is might be a breaking change so, though I need it I would like to keep it as small as possible. Of course happy to discuss if this is a blocker to you. I would like to do all of that in one go. It will break external plugins making use of services! |
DaanHoogland
commented
Apr 28, 2017
ContributorAuthor
the virtio failures are addressed in #2066. still needs a 'this reads like poetry to me" |
rafaelweingartner
commented
Apr 28, 2017
Member
@DaanHoogland for me it is the same, I just thought that every time we create or move something to a new/different package, we could try to use the standard "org.apache….". I did not say that this is something that must be done. From my perspective, you are already creating the package "com.cloud.network.tls", right? With a can of worms, we can always get some fishes ;) |
ContributorAuthor
falling for the presure from my respected peer @rafaelweingartner with an a-umlaut; left as an exercise to the reader: worms to be found when related packages are moved ;) |
DaanHoogland
commented
May 1, 2017
ContributorAuthor
@blueorangutan package |
blueorangutan
commented
May 1, 2017
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
May 1, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-691 |
DaanHoogland
commented
May 1, 2017
ContributorAuthor
@blueorangutan test |
blueorangutan
commented
May 1, 2017
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
rafaelweingartner
left a comment
Member
There was a problem hiding this comment.
@DaanHoogland I have a doubt on a package changes, could you help me understand them?
| <property name="gslbServiceProviders" value="#{gslbServiceProvidersRegistry.registered}" /> | ||
| </bean> | ||
| <bean id="certServiceImpl" class="org.apache.cloudstack.network.lb.CertServiceImpl" /> | ||
| <bean id="certServiceImpl" class="org.apache.cloudstack.network.ssl.CertServiceImpl" /> |
There was a problem hiding this comment.
shouldn't this package be "org.apache.cloudstack.network.tls"?
ContributorAuthor
There was a problem hiding this comment.
could be but that would in this state be incorrect (see other discuss location in this review thread.
Member
There was a problem hiding this comment.
@karuturi the interfaces are in 'tls' sub-package, but the impl are still in 'ssl'?
ContributorAuthor
There was a problem hiding this comment.
@rhtyd we will address that when we make this into a CA-service. I'll create a ticket. It is really very minor.
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| package org.apache.cloudstack.network.lb; | ||
| package org.apache.cloudstack.network.ssl; |
There was a problem hiding this comment.
I got a little confuse here.
Did you create "org.apache.cloudstack.network.tls" or "org.apache.cloudstack.network.ssl"?
ContributorAuthor
There was a problem hiding this comment.
ae4e571#diff-ebb55a3f7379d7132877a94057342689R39 shows "org.apache.cloudstack.network.tls". I will have a quick check to see what happened locally?
ContributorAuthor
There was a problem hiding this comment.
So here is what happened, @rafaelweingartner :
The service definition was in cloud.com... the service implementation was (already) in org.apache.cloudstack...
then I moved the definition around because of your and @resmo 's comments.
Now I could of course take a few seconds to move this code but I won't right now. I am in the process of building on it and will get to that in a next iteration.
There was a problem hiding this comment.
Ah, now I get it. So, someone else in the past moved the implementation, but not the interfaces.
That is why I was confused, thanks for the explanation and the good work 👍
rafaelweingartner
commented
May 1, 2017
Member
@DaanHoogland ahahah, no pressure man :), |
DaanHoogland
commented
May 1, 2017
ContributorAuthor
Don't worry @rafaelweingartner I have an autoignoreonpressure mode. |
blueorangutan
commented
May 2, 2017
Trillian test result (tid-1054)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this can be used in ApplicationClusters as well as in planned CA-plugins