AJ-1234: Use helmchart name for pact identifier.

[jladieu]

This is prefactoring work for AJ-1234

This pairs with DataBiosphere/terra-workspace-data-service#384, where wds also renames its reference from sam-provider to just sam.

• See thread discussing the -consumer and -provider suffix anti-pattern.
• See thread discussing the recommendation to use the helm chart name.
• See thread discussing doing this for sam.

[Ghost-in-a-Jar]

sam also can be a consumer, will this cause any name collisions/issues with that?

[jladieu]

I think ultimately what we want is for sam to self-identify itself as sam when acting in both the provider and consumer role. I think this will let the pact-broker do some dependency graph magic which is currently stymied by having the consumer/provider roles named divergently.

So I think there may be some followup work to rename the consumer side of things as well, but I wanted to try to keep the scope of this change as tight as possible.

[Ghost-in-a-Jar]

ok nice that makes sense thanks! btw side-note the verify-consumer-pact action was failing on our pr's and it looks like this change fixed it...?

[jladieu]

Oh dang, I didn't realize these were failing previously.

And, I hate to disappoint... but I think that's actually a false 'pass' on this PR...

The first time a pact verification runs for a given provider (in this case, the newly renamed sam) it treats the contracts as being in a 'pending' state, and quietly passes the tests (even if they're failing).

You need to go to the logs to see that the tests are still failing with the same looking stack traces as before this PR.

[jladieu]

Took a quick look at the failure @Ghost-in-a-Jar and I think it's because the verify_consumer_pacts.yml needs to somehow run /bin/bash -c env/test.env as part of setting up its environment before running sbt test on the SamProviderSpec.

I tried a few different incantations (see 5d547ce for latest attempt) but kept hitting permission problems, so I am backing that commit out.

[Ghost-in-a-Jar]

ok good to know thanks for taking a look!

[Ghost-in-a-Jar]

@jladieu I managed to fix it with this change https://github.com/broadinstitute/sam/pull/1233/files#diff-88f8d59210f317b7bdd12e6a807c0b84cf473372221acb3ce860f5a0160183b1

[jladieu]

Perfect, thanks! I picked up that change and will see how re-running looks.

[jladieu]

@Ghost-in-a-Jar I think you fixed the issue with permissions, but it looks like maybe another false pass on the PR you linked: If you take a look at: https://github.com/broadinstitute/sam/actions/runs/6711027848/job/18237474362 I'm still seeing some errors (despite the GHA passing).

Example:

    1.1) body: $.userEmail Expected '[email protected]' (String) but received '[email protected]' (String)

    1.2) body: $.userSubjectId Expected 'testUser' (String) but received '509175583607142998ceb' (String)

The bright side is the errors look like they're a lot easier to troubleshoot than the one you fixed since they're concrete pact problems rather than connectivity issues.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication