Add Artifacts Manifest Extension #1649

Timthetic · 2024-05-24T07:22:38Z

Before this PR

Automatic discovery of the "main container image" has been deprecated for a while now.

After this PR

==COMMIT_MSG==
Explicitly list main container image in artifacts extension
==COMMIT_MSG==

Possible downsides?

changelog-app · 2024-05-24T07:22:42Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

add artifacts manifest extension

Check the box to generate changelog(s)

Generate changelog entry

iamdanfox · 2024-05-24T20:49:54Z

changelog/@unreleased/pr-1649.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: add artifacts manifest extension


nice - could we include a mention (and maybe example) in the README so that if people search for this they'll find something?

iamdanfox

Looks neat to me! Just need a real +1 from @CRogers / other BE infra maintainer

Invalidated by push of 5180fc0

felixdesouza

Thanks for putting this together.

A few comments below.

Can you also add a test to CreateManifestTaskIntegrationSpec, so we can be sure that it works when invoked via the buildscript? It will also ensure that we don't inadvertently break this feature in the future.

felixdesouza · 2024-05-24T21:01:33Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/tasks/CreateManifestTask.java

@@ -154,6 +163,7 @@ final void createManifest() throws IOException {
                        .productVersion(getProjectVersion())
                        .putAllExtensions(getManifestExtensions().get())
                        .putExtensions("product-dependencies", productDependencies)
+                        .putExtensions("artifacts", artifacts)


as a safety mechanism, I think we should check to see if there is any artifacts extension currently, and then throw telling users to use the new thing instead of silently failing. As whatever they put before will be clobbered.

felixdesouza · 2024-05-29T11:43:40Z

readme.md

+
+```gradle
+distribution {
+   artifact {


is it artifacts or artifact ? going from the above statement:

You can specify container images that your product requires using the artifacts declaration on the distribution

felixdesouza · 2024-05-29T11:50:29Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/BaseDistributionExtension.java

@@ -75,6 +77,7 @@ public BaseDistributionExtension(Project project) {
        productDependencies = project.getObjects().listProperty(ProductDependency.class);
        optionalProductDependencies = project.getObjects().setProperty(ProductId.class);
        ignoredProductDependencies = project.getObjects().setProperty(ProductId.class);
+        artifacts = project.getObjects().setProperty(ArtifactLocator.class);


let's make this a DomainObjectSet. You can get an instance of one via project.getObjects().domainObjectSet(). It allows you to have listeners on things and some convenient methods which I will discuss below.

you would also set up your configureEach/whenObjectAdded here.

felixdesouza · 2024-05-29T11:51:43Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/artifacts/ArtifactLocator.java

+        uriIsValid(uri);
+    }
+
+    public static void uriIsValid(String uri) {


nit: private

felixdesouza · 2024-05-29T11:54:06Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/BaseDistributionExtension.java

+    }
+
+    public final void artifact(ArtifactLocator artifactLocator) {
+        artifactLocator.isValid();


I don't believe you want to do this here, because there's nothing stopping people from using the getArtifacts() method and adding to the set directly.

You can either:

remove direct access to getArtifacts() by making it private/package-private - although this might not be foolproof considering that gradle buildscripts can access things

or, use a configureEach or whenObjectAdded on the named domain object set. Then it doesn't matter how ArtifactLocator ends up in the set, it will then be validated before anyone can do anything with it.

felixdesouza · 2024-05-29T12:11:06Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/artifacts/ArtifactLocator.java

+import java.util.Objects;
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+public final class ArtifactLocator implements Serializable {


You should make this a managed type. This will allow you to have lazily set properties and not have to get/set in an afterEvaluate in your consumers. Yes, it is different from all the others, however, it's a bit of a pain to migrate all the others.

This will crop up in your PR when you try to use this new thing.

You will then have to have a separate type for json serialization, but you won't have to expose that to consumers.

Huh, looks like the docs aren't there for Gradle 8.8. For myself, I'm linking them for 7.6.2: https://docs.gradle.org/7.6.2/userguide/custom_gradle_types.html#managed_types

felixdesouza · 2024-05-29T12:17:14Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/BaseDistributionExtension.java

+
+    /** Lazily configures and adds a {@link ArtifactLocator}. */
+    public final void artifact(@DelegatesTo(ArtifactLocator.class) Closure<ArtifactLocator> closure) {
+        artifacts.add(providerFactory.provider(() -> {


this simplifies to just:

create the ArtifactLocator

You will need to do something like project.getObjects().newInstance(ArtifactLocator.class) to make Gradle instantiate and "managed" your managed type for you.

configure it

can just do your project.configure(artifactLocator, closure), and you don't have to worry about wrapping that in a provider since the underlying type is lazy

just add it

add it to the domain object set, and the validation in the configureEach or whenObjectAdded will take over.

you should also add a artifact(Action<ArtifactLocator> action) { ... } method. That gives you a type safe way of adding it to the set when using Java. Buildscript will likely pick the Closure form.

Instead of project.configure(...) you end up just doing action.execute(locator).

felixdesouza · 2024-05-29T12:18:39Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/BaseDistributionExtension.java

+        return artifacts;
+    }
+
+    public final void artifact(ArtifactLocator artifactLocator) {


With the whole managed type thing, it becomes a lot more cumbersome for consumers to make this as it needs to be instantiated via gradle. So you should get rid of this method and make them use the closure/action methods below.

if we go down the whole 'managed type' route, is it still convenient to interact with these APIs from other java codebases??

yes, it's how all new(er) plugins are written and also preferred way from Gradle. You have all the lazy safe mutators via map and flatMap etc.

As argued in some other comment, not having Property means that you can never be sure when the value of things inside ArtifactLocator is finally set. Depending on when your plugin is applied and when someone or something might change the value that ArtifactLocator depends on, means that you often end up having to write afterEvaluate which mostly sucks.

felixdesouza · 2024-05-29T12:24:52Z

readme.md

+```gradle
+distribution {
+   artifact {
+        type = 'oci'


is there any point of having the user specify oci ? is it always oci?

Regardless of whether I think we include it or not. We should document what other options there are and what effect it has.

I added some notes about this in the readme, but the short answer is that I wanted to match what we do in the SLS spec. I'd prefer being a little verbose over baking in an assumption that "oci" will be the only type we ever support

felixdesouza · 2024-05-29T12:26:49Z

readme.md

+}
+```
+
+The result will be embedded in the `deployment/manifest.yml` file.


For manifest consumers, it might be worth giving a snippet of what this does to the manifest.yml file. Doesn't have to be the whole file, just the basic required ones and what it adds to the extensions object or whatever it's supposed to be.

felixdesouza · 2024-06-12T16:49:42Z

...ging/src/test/groovy/com/palantir/gradle/dist/tasks/CreateManifestTaskIntegrationSpec.groovy

+    def 'write artifacts to manifest'() {
+        buildFile << """
+        distribution {
+            ${ARTIFACT}


please inline

felixdesouza · 2024-06-12T16:49:58Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/tasks/CreateManifestTask.java

+    private void validateEmptyArtifactsExtension() {
+        Preconditions.checkArgument(
+                !getManifestExtensions().get().containsKey("artifacts"),
+                "Artifacts "
+                        + "specified directly the using the manifest-extensions block in the 'distributions' "
+                        + "extension will be overwritten! Please use the 'artifact' closure in the 'distributions' "
+                        + "extension to add artifacts instead.");
+    }
+


talked offline, please rephrase

felixdesouza · 2024-06-12T16:54:32Z

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/artifacts/JsonArtifactLocator.java

+import java.util.Objects;
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+public final class JsonArtifactLocator implements Serializable {


offline: get rid of serializable and don't use this in the gradle type

…sion

Invalidated by push of b1aacdf

felixdesouza · 2024-06-13T21:06:22Z

👍

write default image to artifacts extension

339afe3

Set

9c2e04d

Timthetic closed this May 24, 2024

just the extension

d0ebead

Timthetic reopened this May 24, 2024

Timothy Rediehs and others added 3 commits May 24, 2024 11:11

Add method that accepts closure

d2f8397

Add generated changelog entries

f82a3b1

Add generated changelog entries

8f8622f

Timthetic marked this pull request as ready for review May 24, 2024 18:16

Timthetic changed the title ~~[WIP] Write main container image in artifacts extension~~ Add Artifacts Manifest Extension May 24, 2024

iamdanfox reviewed May 24, 2024

View reviewed changes

iamdanfox previously approved these changes May 24, 2024

View reviewed changes

readme

5180fc0

felixdesouza requested changes May 29, 2024

View reviewed changes

CR feedback

9bfa604

felixdesouza reviewed Jun 12, 2024

View reviewed changes

Timothy Rediehs added 2 commits June 12, 2024 10:44

update

ef92984

Normal version

74e3af4

felixdesouza previously approved these changes Jun 13, 2024

View reviewed changes

Merge branch 'develop' into trediehs/default-image-to-artifacts-exten…

b1aacdf

…sion

felixdesouza added the merge when ready label Jun 13, 2024

bulldozer-bot bot merged commit cef592c into develop Jun 13, 2024
5 checks passed

bulldozer-bot bot deleted the trediehs/default-image-to-artifacts-extension branch June 13, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Artifacts Manifest Extension #1649

Add Artifacts Manifest Extension #1649

Timthetic commented May 24, 2024 •

edited

Loading

changelog-app bot commented May 24, 2024 •

edited by Timthetic

Loading

iamdanfox May 24, 2024

iamdanfox left a comment

felixdesouza left a comment

felixdesouza May 24, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

Timthetic Jun 1, 2024 •

edited

Loading

felixdesouza May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

iamdanfox May 29, 2024

felixdesouza May 29, 2024

felixdesouza May 29, 2024

Timthetic Jun 12, 2024

felixdesouza May 29, 2024

felixdesouza Jun 12, 2024

felixdesouza Jun 12, 2024

felixdesouza Jun 12, 2024

felixdesouza commented Jun 13, 2024

Add Artifacts Manifest Extension #1649

Add Artifacts Manifest Extension #1649

Conversation

Timthetic commented May 24, 2024 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented May 24, 2024 • edited by Timthetic Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

iamdanfox left a comment

Choose a reason for hiding this comment

felixdesouza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timthetic Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

create the ArtifactLocator

configure it

just add it

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdesouza commented Jun 13, 2024

Timthetic commented May 24, 2024 •

edited

Loading

changelog-app bot commented May 24, 2024 •

edited by Timthetic

Loading

Generate changelog in `changelog/@unreleased`

Timthetic Jun 1, 2024 •

edited

Loading

create the `ArtifactLocator`