Skip to content

Closes #75. Properly set the conditions when call to applyOutputs fails.#101

Merged
karanthukral merged 2 commits intomainfrom
fix-SetConditions-on-applyOutputs-failure
Dec 10, 2025
Merged

Closes #75. Properly set the conditions when call to applyOutputs fails.#101
karanthukral merged 2 commits intomainfrom
fix-SetConditions-on-applyOutputs-failure

Conversation

@karanthukral
Copy link
Copy Markdown
Contributor

@karanthukral karanthukral commented Dec 3, 2025 •
edited
Loading

Closes #75

Managing the PR process for @dansawyer73

💸 TL;DR

  • Ensure the status condition is updated to be false when ApplyOutputs fails. This ensures that the resource is not marked ready when status conditions fail

🧪 Testing Steps / Validation

  • Couldn't figure out a sane way to test this in envtests without needing to use a fake client. Ended up using manual verification in a couple of internal controllers.
  • Updated the sdk version in a controller locally and ran all tests to ensure the behaviour remains consistent for current users
  • Updated controller to try to create a deployment in a namespace that doesn't exist and the status of the object is shown below:
- lastTransitionTime: "2025-12-04T19:56:45Z"
      message: 'Failed to apply outputs: ensuring outputs: ensuring /, Kind= broken-deployment:
        cannot create object: namespaces "namespace-does-not-exist" not found'
      observedGeneration: 7
      reason: ApplyOutputsFailed
      status: "False"
      type: ContainerPrefixFetched
    - lastTransitionTime: "2025-12-04T19:56:45Z"
      message: 'Non-successful conditions: ContainerPrefixFetched'
      observedGeneration: 7
      reason: ConditionsFailed
      status: "False"
      type: Ready

If anyone has ideas on how best to test this in env/unit tests let me know

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee

@karanthukral
Copy link
Copy Markdown
Contributor Author

karanthukral commented Dec 3, 2025

Created a draft PR that I will get tested tomorrow hopefully and opened for actual review

@karanthukral karanthukral marked this pull request as ready for review December 4, 2025 20:23
@karanthukral karanthukral requested a review from a team as a code owner December 4, 2025 20:23
kdorosh
kdorosh reviewed Dec 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@kdorosh kdorosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one question for my own understanding

pkg/fsm/internal/reconciler.go

if err := r.applyOutputs(ctx, log, obj, out); err != nil {
// Mark the state's condition as failed since outputs couldn't be applied
if !condition.IsEmpty() {
Copy link
Copy Markdown
Contributor

@kdorosh kdorosh Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do this even if the condition isn't empty?

Copy link
Copy Markdown
Contributor Author

@karanthukral karanthukral Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question, let me think about it. The only weird part of that would be what is the condition type since we are overriding other fields here? Should it just be empty? 🤔

The IsEmpty returns true only if all fields are empty - https://github.com/reddit/achilles-sdk-api/blob/main/api/condition.go#L115

Copy link
Copy Markdown
Contributor Author

@karanthukral karanthukral Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also seem to do this same check when setting any other fields for the condition too -

achilles-sdk/pkg/fsm/internal/reconciler.go

Line 276 in 04c816e

if !condition.IsEmpty() {

Copy link
Copy Markdown
Contributor

@kdorosh kdorosh Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think the change as you have it is safer / less intrusive

kdorosh
kdorosh approved these changes Dec 8, 2025
View reviewed changes
pkg/fsm/internal/reconciler.go

if err := r.applyOutputs(ctx, log, obj, out); err != nil {
// Mark the state's condition as failed since outputs couldn't be applied
if !condition.IsEmpty() {
Copy link
Copy Markdown
Contributor

@kdorosh kdorosh Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think the change as you have it is safer / less intrusive

@karanthukral karanthukral merged commit 9004e0f into main Dec 10, 2025
1 check passed
@karanthukral karanthukral deleted the fix-SetConditions-on-applyOutputs-failure branch December 10, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jessicayuen jessicayuen jessicayuen approved these changes

@kdorosh kdorosh kdorosh approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ready status is true when outputs fail to apply

3 participants

@karanthukral @jessicayuen @kdorosh