Skip to content

Commit

Permalink
refactor(kubernetes): Fix a few warning types (#4823)
Browse files Browse the repository at this point in the history
* refactor(kubernetes): Apply type bounds to implementations of AtomicOperation

Now that the interfaces have type bounds, we can apply these to the implementations
as well.

* refactor(kubernetes): Suppress some unchecked warnings

KubernetesManifest is full of unchecked casts; let's just add
a comment to the class and suppress the warnings for any methods
that currently have unchecked casts.

As noted in the comment, we should really try to stop relying on
this class that extends HashMap and is full of these unsafe casts,
but for now we'll just suppress the warnings.

I have not enabled the Xlint:unchecked flag because there are still
a few other unchecked warnings that I'll leave for later.

Also let's make computedKind transient because the class implements
Serializable. It doesn't really matter because we don't use java
serialization, but is more correct because this is just a cached
value that is stored to avoid re-computing.

* fix(kubernetes): Fix deprecation warnings

This commit fixes the small number of deprecation warnings in
clouddriver-kubernetes and enables deprecation compiler warnings.

Notes:
* getPointCoordinates was marked deprecated, but is still actively
used in the Delete Manifest stage; there appers to have been no
effort to actually deprecate it, so let's just remove the annotation.
* Stop logging the full manifest when we hit the warning on
that deprecated call; just the identifying info should be enough.
* getRequiredGroupMembership is a field; to add the suppress
annotation we need to write the lombok-generated accessor
explicitly

Also two small other fixes I noticed:
* Make a field private
* Remove Serializable from KubernetesV2Job status as we never use
Java serialization.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
ezimanyi and mergify[bot] committed Aug 20, 2020
1 parent 6588646 commit aeea8e4
Show file tree
Hide file tree
Showing 44 changed files with 108 additions and 61 deletions.
1 change: 0 additions & 1 deletion clouddriver-kubernetes/clouddriver-kubernetes.gradle
Expand Up @@ -21,7 +21,6 @@ tasks.withType(JavaCompile) {

// Temporarily suppressed warnings. These are here only while we fix or
// suppress the warnings in these categories.
'-Xlint:-deprecation',
'-Xlint:-rawtypes',
'-Xlint:-unchecked',
]
Expand Down
Expand Up @@ -23,6 +23,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.caching.view.provider.ArtifactProvider;
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.artifact.KubernetesCleanupArtifactsDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.op.OperationResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.artifact.KubernetesCleanupArtifactsOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand All @@ -37,12 +38,12 @@ public class KubernetesCleanupArtifactsConverter
@Autowired ArtifactProvider artifactProvider;

@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<OperationResult> convertOperation(Map<String, Object> input) {
return new KubernetesCleanupArtifactsOperation(convertDescription(input), artifactProvider);
}

@Override
public KubernetesCleanupArtifactsDescription convertDescription(Map input) {
public KubernetesCleanupArtifactsDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesCleanupArtifactsDescription.class);
}
Expand Down
Expand Up @@ -23,6 +23,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.caching.view.provider.KubernetesV2ArtifactProvider;
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.job.KubernetesRunJobOperationDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.op.job.KubernetesRunJobDeploymentResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.job.KubernetesRunJobOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand All @@ -46,12 +47,13 @@ public KubernetesRunJobOperationConverter(
}

@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<KubernetesRunJobDeploymentResult> convertOperation(
Map<String, Object> input) {
return new KubernetesRunJobOperation(convertDescription(input), artifactProvider, appendSuffix);
}

@Override
public KubernetesRunJobOperationDescription convertDescription(Map input) {
public KubernetesRunJobOperationDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesRunJobOperationDescription.class);
}
Expand Down
Expand Up @@ -22,6 +22,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesOperation;
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesDeleteManifestDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.op.OperationResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesDeleteManifestOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand All @@ -32,12 +33,12 @@
@Component
public class KubernetesDeleteManifestConverter extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<OperationResult> convertOperation(Map<String, Object> input) {
return new KubernetesDeleteManifestOperation(convertDescription(input));
}

@Override
public KubernetesDeleteManifestDescription convertDescription(Map input) {
public KubernetesDeleteManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesDeleteManifestDescription.class);
}
Expand Down
Expand Up @@ -26,6 +26,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesDeployManifestDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesManifest;
import com.netflix.spinnaker.clouddriver.kubernetes.op.OperationResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesDeployManifestOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand Down Expand Up @@ -58,12 +59,12 @@ public KubernetesDeployManifestConverter(
}

@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<OperationResult> convertOperation(Map<String, Object> input) {
return new KubernetesDeployManifestOperation(convertDescription(input), artifactProvider);
}

@Override
public KubernetesDeployManifestDescription convertDescription(Map input) {
public KubernetesDeployManifestDescription convertDescription(Map<String, Object> input) {
KubernetesDeployManifestDescription mainDescription =
KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesDeployManifestDescription.class);
Expand Down
Expand Up @@ -22,6 +22,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesOperation;
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesEnableDisableManifestDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.op.OperationResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesDisableManifestOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand All @@ -32,12 +33,12 @@
@Component
public class KubernetesDisableManifestConverter extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<OperationResult> convertOperation(Map<String, Object> input) {
return new KubernetesDisableManifestOperation(convertDescription(input));
}

@Override
public KubernetesEnableDisableManifestDescription convertDescription(Map input) {
public KubernetesEnableDisableManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesEnableDisableManifestDescription.class);
}
Expand Down
Expand Up @@ -22,6 +22,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesOperation;
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesEnableDisableManifestDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.op.OperationResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesEnableManifestOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand All @@ -32,12 +33,12 @@
@Component
public class KubernetesEnableManifestConverter extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<OperationResult> convertOperation(Map<String, Object> input) {
return new KubernetesEnableManifestOperation(convertDescription(input));
}

@Override
public KubernetesEnableDisableManifestDescription convertDescription(Map input) {
public KubernetesEnableDisableManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesEnableDisableManifestDescription.class);
}
Expand Down
Expand Up @@ -22,6 +22,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesOperation;
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.converters.KubernetesAtomicOperationConverterHelper;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesPatchManifestDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.op.OperationResult;
import com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesPatchManifestOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
Expand All @@ -32,12 +33,12 @@
@KubernetesOperation(PATCH_MANIFEST)
public class KubernetesPatchManifestConverter extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<OperationResult> convertOperation(Map<String, Object> input) {
return new KubernetesPatchManifestOperation(convertDescription(input));
}

@Override
public KubernetesPatchManifestDescription convertDescription(Map input) {
public KubernetesPatchManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesPatchManifestDescription.class);
}
Expand Down
Expand Up @@ -33,12 +33,12 @@
public class KubernetesPauseRolloutManifestConverter
extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<Void> convertOperation(Map<String, Object> input) {
return new KubernetesPauseRolloutManifestOperation(convertDescription(input));
}

@Override
public KubernetesPauseRolloutManifestDescription convertDescription(Map input) {
public KubernetesPauseRolloutManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesPauseRolloutManifestDescription.class);
}
Expand Down
Expand Up @@ -33,12 +33,12 @@
public class KubernetesResumeRolloutManifestConverter
extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<Void> convertOperation(Map<String, Object> input) {
return new KubernetesResumeRolloutManifestOperation(convertDescription(input));
}

@Override
public KubernetesResumeRolloutManifestDescription convertDescription(Map input) {
public KubernetesResumeRolloutManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesResumeRolloutManifestDescription.class);
}
Expand Down
Expand Up @@ -33,12 +33,12 @@
public class KubernetesRollingRestartManifestConverter
extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<Void> convertOperation(Map<String, Object> input) {
return new KubernetesRollingRestartManifestOperation(convertDescription(input));
}

@Override
public KubernetesRollingRestartManifestDescription convertDescription(Map input) {
public KubernetesRollingRestartManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesRollingRestartManifestDescription.class);
}
Expand Down
Expand Up @@ -32,12 +32,12 @@
@Component
public class KubernetesScaleManifestConverter extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<Void> convertOperation(Map<String, Object> input) {
return new KubernetesScaleManifestOperation(convertDescription(input));
}

@Override
public KubernetesScaleManifestDescription convertDescription(Map input) {
public KubernetesScaleManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesScaleManifestDescription.class);
}
Expand Down
Expand Up @@ -33,12 +33,12 @@
public class KubernetesUndoRolloutManifestConverter
extends AbstractAtomicOperationsCredentialsSupport {
@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<Void> convertOperation(Map<String, Object> input) {
return new KubernetesUndoRolloutManifestOperation(convertDescription(input));
}

@Override
public KubernetesUndoRolloutManifestDescription convertDescription(Map input) {
public KubernetesUndoRolloutManifestDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesUndoRolloutManifestDescription.class);
}
Expand Down
Expand Up @@ -34,12 +34,12 @@ public class KubernetesResizeServerGroupConverter
extends AbstractAtomicOperationsCredentialsSupport {

@Override
public AtomicOperation convertOperation(Map input) {
public AtomicOperation<Void> convertOperation(Map<String, Object> input) {
return new KubernetesResizeServerGroupOperation(convertDescription(input));
}

@Override
public KubernetesResizeServerGroupDescription convertDescription(Map input) {
public KubernetesResizeServerGroupDescription convertDescription(Map<String, Object> input) {
return KubernetesAtomicOperationConverterHelper.convertDescription(
input, this, KubernetesResizeServerGroupDescription.class);
}
Expand Down
Expand Up @@ -25,7 +25,7 @@

public class KubernetesAtomicOperationConverterHelper {
public static <T extends KubernetesAtomicOperationDescription> T convertDescription(
Map input,
Map<String, Object> input,
AbstractAtomicOperationsCredentialsSupport credentialsSupport,
Class<T> targetDescriptionType) {
String account = (String) input.get("account");
Expand Down
Expand Up @@ -32,7 +32,7 @@
@EqualsAndHashCode(callSuper = true)
@Data
public class KubernetesDeleteManifestDescription extends KubernetesAtomicOperationDescription {
V1DeleteOptions options;
private V1DeleteOptions options;
private String manifestName;
private String location;
private List<String> kinds = new ArrayList<>();
Expand All @@ -55,7 +55,6 @@ public List<KubernetesCoordinates> getAllCoordinates() {
}

@JsonIgnore
@Deprecated
public KubernetesCoordinates getPointCoordinates() {
return KubernetesCoordinates.builder()
.namespace(location)
Expand Down
Expand Up @@ -37,11 +37,18 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Because this class maps the received Kubernetes manifest to an untyped map, it has no choice but
* to perform many unchecked casts when retrieving information. New logic should convert the
* manifest to an appropriate strongly-typed model object instead of adding more unchecked casts
* here. Methods that already perform unchecked casts are annotated to suppress them; please avoid
* adding more such methods if at all possible.
*/
public class KubernetesManifest extends HashMap<String, Object> {
private static final Logger log = LoggerFactory.getLogger(KubernetesManifest.class);
private static final ObjectMapper mapper = new ObjectMapper();

@Nullable private KubernetesKind computedKind;
@Nullable private transient KubernetesKind computedKind;

@Override
public KubernetesManifest clone() {
Expand Down Expand Up @@ -98,6 +105,7 @@ public void setApiVersion(KubernetesApiVersion apiVersion) {
}

@JsonIgnore
@SuppressWarnings("unchecked")
private Map<String, Object> getMetadata() {
return Optional.ofNullable((Map<String, Object>) get("metadata"))
.orElseThrow(() -> MalformedManifestException.missingField(this, "metadata"));
Expand Down Expand Up @@ -172,6 +180,7 @@ public List<OwnerReference> getOwnerReferences() {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public KubernetesManifestSelector getManifestSelector() {
if (!containsKey("spec")) {
return null;
Expand All @@ -192,6 +201,7 @@ public KubernetesManifestSelector getManifestSelector() {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public Map<String, String> getLabels() {
Map<String, String> result = (Map<String, String>) getMetadata().get("labels");
if (result == null) {
Expand All @@ -203,6 +213,7 @@ public Map<String, String> getLabels() {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public Map<String, String> getAnnotations() {
Map<String, String> result = (Map<String, String>) getMetadata().get("annotations");
if (result == null) {
Expand All @@ -214,6 +225,7 @@ public Map<String, String> getAnnotations() {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public Double getReplicas() {
if (!containsKey("spec")) {
return null;
Expand All @@ -227,6 +239,7 @@ public Double getReplicas() {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public void setReplicas(Double replicas) {
if (!containsKey("spec")) {
return;
Expand All @@ -240,6 +253,7 @@ public void setReplicas(Double replicas) {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public Optional<Map<String, String>> getSpecTemplateLabels() {
if (!containsKey("spec")) {
return Optional.empty();
Expand Down Expand Up @@ -274,6 +288,7 @@ public Optional<Map<String, String>> getSpecTemplateLabels() {
}

@JsonIgnore
@SuppressWarnings("unchecked")
public Optional<Map<String, String>> getSpecTemplateAnnotations() {
if (!containsKey("spec")) {
return Optional.empty();
Expand Down
Expand Up @@ -17,14 +17,19 @@

package com.netflix.spinnaker.clouddriver.kubernetes.description.manifest;

import com.netflix.spinnaker.clouddriver.kubernetes.description.KubernetesCoordinates;
import com.netflix.spinnaker.clouddriver.kubernetes.security.KubernetesCredentials;

public class KubernetesSourceCapacity {

public static Double getSourceCapacity(
KubernetesManifest manifest, KubernetesCredentials credentials) {
KubernetesManifest currentManifest =
credentials.get(manifest.getKind(), manifest.getNamespace(), manifest.getName());
credentials.get(
KubernetesCoordinates.builder()
.kind(manifest.getKind())
.namespace(manifest.getNamespace())
.name(manifest.getName())
.build());
if (currentManifest != null) {
return currentManifest.getReplicas();
}
Expand Down

0 comments on commit aeea8e4

Please sign in to comment.