Alon Bar-Lev has posted comments on this change.

Change subject: utils: pki-resource: cleanup: levarage enum methods for content 
type
......................................................................


Patch Set 5:

(4 comments)

Hi,

I am very sorry... but it does not makes it any simpler, just more complex.

It should be exact opposite... having the resources as enum not the formats, 
and allow get the format based on specific format.

You also created dependency between servlet interface and internal 
representation of enum, which is bad especially in java environment in which 
every programmer can just rename enums and breaks application.

So although I appreciate your effort, I will leave this as-is for now.

Thanks!

I will reuse the servlet utils though.

Alon

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetCACertificateQuery.java
Line 12:     protected void executeQueryCommand() {
Line 13:         try {
Line 14:             getQueryReturnValue().setSucceeded(false);
Line 15:             getQueryReturnValue().setReturnValue(
Line 16:                 PKIResources.X509_PEM.getAsString()
how do you select which certificate?
Line 17:             );
Line 18:             getQueryReturnValue().setSucceeded(true);
Line 19:         } catch (Exception e) {
Line 20:             getQueryReturnValue().setExceptionString(e.getMessage());


....................................................
File backend/manager/modules/root/src/main/webapp/WEB-INF/web.xml
Line 19:         <param-value>/pki-resource</param-value>
Line 20:     </init-param>
Line 21:     <init-param>
Line 22:         <param-name>attr-resource</param-name>
Line 23:         <param-value>X509_PEM_CA</param-value>
you cannot change this, you break the interface of installer. there cannot be 
dependency between interface and internal enum names.
Line 24:     </init-param>
Line 25:   </servlet>
Line 26:   <servlet-mapping>
Line 27:     <servlet-name>PKIResourceServlet.ca</servlet-name>


....................................................
File 
backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/PKIResourceServlet.java
Line 42:             }
Line 43:             PKIResources r = PKIResources.valueOf(resourceString);
Line 44:             Format format = r.getFormat();
Line 45:             if (formatString != null) {
Line 46:                 format = Format.valueOf(formatString);
if someone "refactor" the java, it will break the entire application, as the 
servlet interface will change and the current issued resources will be broken.
Line 47:             }
Line 48: 
Line 49:             if (alias == null) {
Line 50:                 alias = r.getAlias();


....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/PKIResources.java
Line 25:  * </pre>
Line 26:  */
Line 27: public enum PKIResources {
Line 28: 
Line 29:     X509_PEM_CA("application/x-x509-cert", null, 
getCertificate(EngineLocalConfig.getInstance().getPKICACert()), Format.PEM),
the X509_PEM_CA is format it is not specific certificate... I should be able to 
get any certificate in any format.

you can have entry for engine-ca in X509_PEM format, engine-ca in X509_PEM_CA 
format, etc... but this only makes it more complex.
Line 30:     X509_PEM("application/x-x509-ca-cert", null, 
getCertificate(EngineLocalConfig.getInstance().getPKIEngineCert()), Format.PEM),
Line 31:     OPENSSH_PUBKEY("text/plain", "ovirt-engine", 
getCertificate(EngineLocalConfig.getInstance().getPKIEngineCert()), 
Format.OPENSSH);
Line 32: 
Line 33:     private final String contentType;


-- 
To view, visit http://gerrit.ovirt.org/21073
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I24da0ff174599ffdeabbf5846eab429bf0d6510d
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to