Skip to content

Commit

Permalink
refactor(artifacts): Update artifact code to use new metadata accessor (
Browse files Browse the repository at this point in the history
#4645)

* refactor(kubernetes): Avoid mutating artifact metadata

There's only one place where we mutate the artifact metadata
map, and it's in the Kubernetes provider. Rather than get an
artifact then add the metadata later, let's just move the logic
to add that metdata to the place where we create the artifact,
avoiding the need to mutate the metadata.

* refactor(artifacts): Update artifact code to use new metadata accessor

The accessor to get the full metadata of an artifact as a map is deprecated;
replace calls to it with the accessor for a specific key.

The reason the function to return the entire map is deprecated is so that we
can eventually make the map immutable; it's much safer to make that change
if we've first updated callers to be unaware that the map exists at all
an instead give them a function to get the information they want.

In most cases this actually simplifies the calling code as we can remove
null-checks that are now handled by the exposed API function.
  • Loading branch information
ezimanyi committed Jun 2, 2020
1 parent fa39a0f commit b1e586e
Show file tree
Hide file tree
Showing 7 changed files with 13 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import java.io.OutputStream;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Map;
import java.util.UUID;
import lombok.Getter;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -153,13 +152,7 @@ private void archiveToOutputStream(
}

private String artifactSubPath(Artifact artifact) {
String target = "";
Map<String, Object> metadata = artifact.getMetadata();
if (metadata != null) {
target = (String) metadata.getOrDefault("subPath", "");
}

return target;
return Strings.nullToEmpty((String) artifact.getMetadata("subPath"));
}

private String artifactVersion(Artifact artifact) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,7 @@ private static ExternalReference resolveBuildInfo(
Map<String, Object> buildInfo = null;
final Artifact applicationArtifact = description.getApplicationArtifact();
if (applicationArtifact != null) {
final Map<String, Object> metadata = applicationArtifact.getMetadata();
if (metadata != null) {
buildInfo = (Map<String, Object>) applicationArtifact.getMetadata().get("build");
}
buildInfo = (Map<String, Object>) applicationArtifact.getMetadata("build");
}
if (buildInfo == null) {
final Map<String, Object> trigger = description.getTrigger();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import java.io.IOException;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import javax.annotation.Nonnull;
import javax.annotation.ParametersAreNonnullByDefault;
import lombok.Value;
Expand Down Expand Up @@ -95,12 +94,7 @@ private static ImmutableList<Artifact> filterKubernetesArtifactsByNamespaceAndAc
}

private static String getAccount(Artifact artifact) {
String account = "";
Map<String, Object> metadata = artifact.getMetadata();
if (metadata != null) {
account = (String) metadata.getOrDefault("account", "");
}
return account;
return Strings.nullToEmpty((String) artifact.getMetadata("account"));
}

@Nonnull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,7 @@ private Optional<String> findMatchingVersion(
}

private Optional<KubernetesManifest> getLastAppliedConfiguration(Artifact artifact) {
if (artifact.getMetadata() == null) {
return Optional.empty();
}

Object rawLastAppliedConfiguration = artifact.getMetadata().get("lastAppliedConfiguration");
Object rawLastAppliedConfiguration = artifact.getMetadata("lastAppliedConfiguration");

if (rawLastAppliedConfiguration == null) {
return Optional.empty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,13 @@ public class KubernetesCacheDataConverter {
private static Optional<Keys.CacheKey> convertAsArtifact(
KubernetesCacheData kubernetesCacheData, String account, KubernetesManifest manifest) {
String namespace = manifest.getNamespace();
Optional<Artifact> optional = KubernetesManifestAnnotater.getArtifact(manifest);
Optional<Artifact> optional = KubernetesManifestAnnotater.getArtifact(manifest, account);
if (!optional.isPresent()) {
return Optional.empty();
}

Artifact artifact = optional.get();

try {
KubernetesManifest lastAppliedConfiguration =
KubernetesManifestAnnotater.getLastAppliedConfiguration(manifest);
artifact.getMetadata().put("lastAppliedConfiguration", lastAppliedConfiguration);
artifact.getMetadata().put("account", account);
} catch (Exception e) {
log.warn("Unable to get last applied configuration from {}: ", manifest, e);
}

if (artifact.getType() == null) {
log.debug(
"No assigned artifact type for resource "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,19 +176,24 @@ private static void storeAnnotations(Map<String, String> annotations, Artifact a
storeAnnotation(annotations, VERSION, artifact.getVersion());
}

public static Optional<Artifact> getArtifact(KubernetesManifest manifest) {
public static Optional<Artifact> getArtifact(KubernetesManifest manifest, String account) {
Map<String, String> annotations = manifest.getAnnotations();
String type = getAnnotation(annotations, TYPE, new TypeReference<String>() {});
if (Strings.isNullOrEmpty(type)) {
return Optional.empty();
}

KubernetesManifest lastAppliedConfiguration =
KubernetesManifestAnnotater.getLastAppliedConfiguration(manifest);

return Optional.of(
Artifact.builder()
.type(type)
.name(getAnnotation(annotations, NAME, new TypeReference<String>() {}))
.location(getAnnotation(annotations, LOCATION, new TypeReference<String>() {}))
.version(getAnnotation(annotations, VERSION, new TypeReference<String>() {}))
.putMetadata("lastAppliedConfiguration", lastAppliedConfiguration)
.putMetadata("account", account)
.build());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ private List<Artifact> artifactsToDelete(KubernetesManifest manifest) {
}

int maxVersionHistory = optionalMaxVersionHistory.getAsInt();
Optional<Artifact> optional = KubernetesManifestAnnotater.getArtifact(manifest);
Optional<Artifact> optional = KubernetesManifestAnnotater.getArtifact(manifest, accountName);
if (!optional.isPresent()) {
return new ArrayList<>();
}
Expand All @@ -114,8 +114,7 @@ private List<Artifact> artifactsToDelete(KubernetesManifest manifest) {
List<Artifact> artifacts =
artifactProvider
.getArtifacts(artifact.getType(), artifact.getName(), artifact.getLocation()).stream()
.filter(
a -> a.getMetadata() != null && accountName.equals(a.getMetadata().get("account")))
.filter(a -> accountName.equals(a.getMetadata("account")))
.collect(Collectors.toList());

if (maxVersionHistory >= artifacts.size()) {
Expand Down

0 comments on commit b1e586e

Please sign in to comment.