Skip to content

Commit

Permalink
fix(kubernetes): Fix ImmutableEnumChecker and rawtypes warnings (#4853)
Browse files Browse the repository at this point in the history
* fix(kubernetes): Make enum fields final

[ImmutableEnumChecker] enums should be immutable: 'PodPhase' has
non-final field 'message'.

* fix(kubernetes): Fix use of raw types and enable warning

I suppressed one warning because the interface requires using a raw
type.

Also the AccountCredentialsRepository interface returns raw types,
which causes one issue where we try to directly use a lambda on the
stream of results. I updated that to not use a lambda (which I think
implicitly implies (AccountCredentials<?> c) -> c.getName() avoiding
the warning, but we should also consider fixing AccountCredentials
itself.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
ezimanyi and mergify[bot] committed Aug 28, 2020
1 parent 7bcb941 commit 6f45399
Show file tree
Hide file tree
Showing 8 changed files with 20 additions and 35 deletions.
2 changes: 0 additions & 2 deletions clouddriver-kubernetes/clouddriver-kubernetes.gradle
Expand Up @@ -21,15 +21,13 @@ tasks.withType(JavaCompile) {

// Temporarily suppressed warnings. These are here only while we fix or
// suppress the warnings in these categories.
'-Xlint:-rawtypes',
'-Xlint:-unchecked',
]

// Temporarily disable error-prone checks that are generating warnings. These
// are only here while we fix or suppress the warnings in these categories.
options.errorprone.disable(
"EmptyCatch",
"ImmutableEnumChecker",
"MissingOverride",
)
}
Expand Down
Expand Up @@ -102,7 +102,7 @@ private Set<String> synchronizeAccountCredentials() {
KubernetesNamedAccountCredentials credentials =
new KubernetesNamedAccountCredentials(managedAccount, credentialFactory);

AccountCredentials existingCredentials =
AccountCredentials<?> existingCredentials =
accountCredentialsRepository.getOne(managedAccount.getName());

if (existingCredentials == null || !existingCredentials.equals(credentials)) {
Expand Down Expand Up @@ -146,9 +146,11 @@ private void saveToCredentialsRepository(KubernetesNamedAccountCredentials accou
private List<String> getDeletedAccountNames() {
List<String> existingNames =
accountCredentialsRepository.getAll().stream()
.filter(
(AccountCredentials c) -> KubernetesCloudProvider.ID.equals(c.getCloudProvider()))
.map(AccountCredentials::getName)
.filter(c -> KubernetesCloudProvider.ID.equals(c.getCloudProvider()))
// Using a lambda here causes a warning about raw types; the real fix would be to have
// the credentials repository return AccountCredentials<?> instead of an
// AccountCredentials.
.map(c -> c.getName())
.collect(Collectors.toList());

Set<String> newNames =
Expand Down
Expand Up @@ -328,6 +328,9 @@ public boolean handles(OnDemandType type, String cloudProvider) {
}

@Override
// The raw Map is required by the method we're overriding; suppress the warning until the
// interface adds type bounds.
@SuppressWarnings("rawtypes")
public Collection<Map> pendingOnDemandRequests(ProviderCache providerCache) {
if (!handlePendingOnDemandRequests()) {
return ImmutableList.of();
Expand Down
Expand Up @@ -89,7 +89,7 @@ private static <T> T getAnnotation(
return getAnnotation(annotations, key, typeReference, null);
}

private static boolean stringTypeReference(TypeReference typeReference) {
private static boolean stringTypeReference(TypeReference<?> typeReference) {
if (typeReference.getType() == null || typeReference.getType().getTypeName() == null) {
log.warn("Malformed type reference {}", typeReference);
return false;
Expand Down
Expand Up @@ -91,8 +91,8 @@ private enum PodPhase {
Failed(true, "Pod has failed"),
Unknown(true, "Pod phase is unknown");

@Getter private String message;
@Getter private boolean unstable;
@Getter private final String message;
@Getter private final boolean unstable;

PodPhase(boolean unstable, String message) {
this.message = message;
Expand Down
Expand Up @@ -109,7 +109,7 @@ public KubernetesRunJobDeploymentResult operate(List<KubernetesRunJobDeploymentR

KubernetesDeployManifestOperation deployManifestOperation =
new KubernetesDeployManifestOperation(deployManifestDescription, provider);
OperationResult operationResult = deployManifestOperation.operate(new ArrayList());
OperationResult operationResult = deployManifestOperation.operate(new ArrayList<>());
KubernetesRunJobDeploymentResult deploymentResult =
new KubernetesRunJobDeploymentResult(operationResult);
Map<String, List<String>> deployedNames = deploymentResult.getDeployedNamesByLocation();
Expand Down
Expand Up @@ -27,7 +27,6 @@
import com.netflix.spinnaker.clouddriver.security.AccountCredentials;
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsProvider;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import org.slf4j.Logger;
Expand Down Expand Up @@ -65,24 +64,6 @@ public boolean validateNotEmpty(String attribute, Object value) {
return true;
}

public boolean validateSizeEquals(String attribute, Collection items, int size) {
if (items.size() != size) {
reject("size!=" + size, attribute);
return false;
}

return true;
}

private boolean validateNotEmpty(String attribute, String value) {
if (Strings.isNullOrEmpty(value)) {
reject("empty", attribute);
return false;
}

return true;
}

public boolean validateCredentials(
AccountCredentialsProvider provider, String accountName, KubernetesManifest manifest) {
KubernetesKind kind = manifest.getKind();
Expand All @@ -96,15 +77,16 @@ public boolean validateCredentials(
KubernetesKind kind,
String namespace) {
log.info("Validating credentials for {} {} {}", accountName, kind, namespace);
if (!validateNotEmpty("account", accountName)) {
if (Strings.isNullOrEmpty(accountName)) {
reject("empty", "account");
return false;
}

if (Strings.isNullOrEmpty(namespace)) {
return true;
}

AccountCredentials credentials = provider.getCredentials(accountName);
AccountCredentials<?> credentials = provider.getCredentials(accountName);
if (credentials == null) {
reject("notFound", "account");
return false;
Expand Down
Expand Up @@ -167,16 +167,16 @@ private static KubernetesConfigurationProperties.ManagedAccount getManagedAccoun
return managedAccount;
}

private static AccountCredentials nonKubernetesAccount(String name) {
AccountCredentials credentials = mock(AccountCredentials.class);
private static AccountCredentials<?> nonKubernetesAccount(String name) {
AccountCredentials<?> credentials = mock(AccountCredentials.class);
when(credentials.getName()).thenReturn(name);
return credentials;
}

private static AccountCredentialsProvider stubAccountCredentialsProvider(
Iterable<AccountCredentials> accounts) {
Iterable<AccountCredentials<?>> accounts) {
AccountCredentialsRepository accountRepository = new MapBackedAccountCredentialsRepository();
for (AccountCredentials account : accounts) {
for (AccountCredentials<?> account : accounts) {
accountRepository.save(account.getName(), account);
}
return new DefaultAccountCredentialsProvider(accountRepository);
Expand Down

0 comments on commit 6f45399

Please sign in to comment.