Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ArtifactLocator s to have inputs derived from task outputs. #1662

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

felixdesouza
Copy link
Contributor

@felixdesouza felixdesouza commented Jul 1, 2024

Before this PR

When you use whenObjectAdded it will eagerly realise any objects that exist in the provider:

Adds an {@code Action} to be executed when an object is added to this collection.
Like {@link #all(Action)}, this method will cause all objects in this container to be realized.

So if a provider has a task output, it will complain because it's trying to realise it at configuration time, which is not possible.

It turns out that the validation in ArtifactLocator would always call get, so it's never enough to do a configureEach or a whenObjectAdded, as we would eagerly realise it the moment a concrete version was added to the set.

An alternative is to wrap the ArtifactLocator into a provider, however, I believe that will lose task dependency information!

After this PR

==COMMIT_MSG==
Allow ArtifactLocators to have inputs derived from task outputs.
==COMMIT_MSG==

Use @Nested in SetProperty task input for CreateManifest task. The validation happens when converting to JsonArtifactLocator. Add integration tests that allow using derived task output providers.

Possible downsides?

@felixdesouza felixdesouza requested a review from CRogers July 1, 2024 17:08
@changelog-app
Copy link

changelog-app bot commented Jul 1, 2024

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

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Allow ArtifactLocators to have inputs derived from task outputs.

Check the box to generate changelog(s)

  • Generate changelog entry

CRogers
CRogers previously approved these changes Jul 1, 2024
@policy-bot policy-bot bot dismissed CRogers’s stale review July 1, 2024 17:12

Invalidated by push of aebdf07

@sfackler
Copy link
Member

sfackler commented Jul 1, 2024

Is this going to be sufficiently lazy? The case I ran into is one where an artifact property is derived from another task, so the property value can't actually be evaluated until the property's task dependencies have run rather than when the set of artifacts is realized.

@felixdesouza
Copy link
Contributor Author

Is this going to be sufficiently lazy? The case I ran into is one where an artifact property is derived from another task, so the property value can't actually be evaluated until the property's task dependencies have run rather than when the set of artifacts is realized.

I think this should be fine? this is only ever passed into the output of another task, so it'd only be evaluated when trying to evaluate those inputs. It's a bit of a shame (having tighter validation loop).

I'll write an extra integration test to confirm, but feel free to clone and check this against your use case as well, then we can iterate from there.

@felixdesouza felixdesouza changed the title Use configureEach instead of whenObjectAdded for the artifacts block. Allow ArtifactLocators to have inputs derived from task outputs. Jul 2, 2024
@felixdesouza felixdesouza changed the title Allow ArtifactLocators to have inputs derived from task outputs. Allow ArtifactLocator s to have inputs derived from task outputs. Jul 2, 2024
@bulldozer-bot bulldozer-bot bot merged commit 2f5aa21 into develop Jul 2, 2024
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the use-configure-each-instead branch July 2, 2024 17:15
@svc-autorelease
Copy link
Collaborator

Released 7.58.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants