Skip to content

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Aug 23, 2024

Description

During our build, we currently are running Pack again after running tests (which is wrong and fixing it is tracked by #5404). Because of that, and due to a recent change that caused a new Restore operation to be called when sending our tests to Helix, the build is now trying to re-run pack one the package that was restored during test runs, but it is doing so by passing /p:Restore=false /p:Build=false. Currently, our ReplacementText targets only run if you are running the Build target as well, but if you run Pack and skip the Build, then the replacement targets won't run, meaning the new generated package won't have the placeholders replaced, which is what is causing #5396 to happen. This change is making it so that our Replacement targets also are hooked into the Pack target even when Build gets skipped which will workaround the issue. The ultimate fix will be to not try to re-pack the projects after testing, which will be tracked by #5404.

Fixes # #5396

cc: @eerhardt @radical

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 23, 2024
@radical radical added area-engineering-systems infrastructure helix infra engineering repo stuff and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 23, 2024
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Waiting on the internal build for validation.

@joperezr
Copy link
Member Author

Found an issue with the initial approach as that was causing even more packages to get re-built post testing, which was causing other packages to be incorrectly built. This new approach is rolling that back and instead just avoiding causing a rebuild at all in that last pack call by no longer depending on restore when sending the tests to helix

@joperezr
Copy link
Member Author

I have validated with the internal build that this is working as expected. This is ready to go.

@joperezr joperezr enabled auto-merge (squash) August 24, 2024 05:28
@joperezr
Copy link
Member Author

/backport to release/8.2

Copy link
Contributor

Started backporting to release/8.2: https://github.com/dotnet/aspire/actions/runs/10536228450

@joperezr joperezr merged commit 7063d81 into dotnet:main Aug 24, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
@joperezr joperezr deleted the FixTextReplacement branch April 1, 2025 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-engineering-systems infrastructure helix infra engineering repo stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants