Skip to content

Add ring-aware Workday runtime installer - #305

Merged
Surendra Goutham (is-goutham) merged 7 commits into
mainfrom
users/gouthams/pr290-05-workday-installer
Sep 25, 2026
Merged

Surendra Goutham (is-goutham) merged 7 commits into
mainfrom
users/gouthams/pr290-05-workday-installer

Conversation

@is-goutham

Copy link
Copy Markdown
Contributor

Summary

  • add a PAC-based Workday package installer for ESS HR agents
  • select the standalone MOS Workday runtime while retaining the legacy DA package mapping
  • select or create the correct Public/Preprod PAC authentication profile
  • prefer the managed PAC installation and provide its package-feed configuration

Validation

  • python -m pytest tests/scripts/test_install_workday_da_extension.py -q (9 passed)
  • python -m py_compile solutions/ess-maker-skills/scripts/install_workday_da_extension.py
  • git diff --check

Stacked on #303.

Comment thread solutions/ess-maker-skills/scripts/install_workday_da_extension.py Outdated
@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Re-reviewed the current head (83707bf) and verified the auth contract against the managed PAC CLI (2.11.2). Two blockers remain:

  • Preprod passes an unsupported PAC cloud value — solutions/ess-maker-skills/scripts/install_workday_da_extension.py:33-36,173-182 maps the ring to --cloud Preprod, but pac auth create accepts only Public, UsGov, UsGovHigh, UsGovDod, and China. Keep the cloud supported (normally Public) and anchor the profile with --environment <target URL>.
  • Production auth is not tied to the requested environment — install_workday_da_extension.py:134-181 matches any Public profile and omits --environment when creating a new production profile. With multiple Public environments or tenants, the script can select/create the wrong auth context. Match the normalized target URL for production as well and always pass the requested environment when creating the profile.

The earlier Preprod URL-matching change improved disambiguation among existing profiles, but it does not make --cloud Preprod a valid PAC invocation. This PR also remains stacked on unresolved PR 300–303 blockers.

@is-goutham

Copy link
Copy Markdown
Contributor Author

Addressed the latest blockers in 26c76c3: both rings use PAC-supported Public cloud, profile reuse requires the exact normalized environment URL for production and preprod, and auth creation always includes --environment. Validation: 64 targeted tests passed.

@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Correction to my previous PR 305 review

My original statement that Preprod was unsupported was incorrect—the canonical bootstrap contract confirms that it is a supported hidden PAC cloud value. However, the latest update now implements the incorrect recommendation and therefore remains blocked.

At the current head (26c76c3), preprod maps to Public, and exact environment matching plus --environment are applied to both rings. The canonical contract requires:

  • PPE: hidden --cloud Preprod, with an environment anchor when needed.
  • Prod: --cloud Public, with cloud-scoped profile selection and no required environment anchor.

Please restore the Preprod mapping and limit environment anchoring to the Preprod path. Prod should retain cloud-only profile selection and creation.

@is-goutham

Copy link
Copy Markdown
Contributor Author

Confirmed against the canonical ring-aware PAC implementation (556a9ae) and the Preprod environment-disambiguation fix (57b5476). I reverted 26c76c3 in 16d591c: PPE again uses the hidden Preprod cloud with exact environment matching and --environment; Production uses Public with cloud-scoped profile reuse and no required environment anchor. Focused validation: 12 passed in ests/scripts/test_install_workday_da_extension.py.

@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Re-reviewed the latest head (16d591c). The PAC ring regression is fixed: PPE again uses the hidden Preprod cloud with its environment anchor, while Prod uses cloud-scoped Public authentication without requiring an anchor.

I found no remaining critical blocker in this PR's own changes. Good to merge.

@nkemms

nkemms commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Follow-up PR: Consider aligning the standalone script's omitted---ring default with bootstrap's PPE default, or make --ring required. Current setup callers pass the recorded ring explicitly, so this does not block the present PR.

nkemms
nkemms previously approved these changes Sep 24, 2026
@is-goutham
Surendra Goutham (is-goutham) added this pull request to stack #341 September 25, 2026 00:17
@is-goutham
Surendra Goutham (is-goutham) dismissed nkemms’s stale review September 25, 2026 00:30

The merge-base changed after approval.

@is-goutham
Surendra Goutham (is-goutham) force-pushed the users/gouthams/pr290-05-workday-installer branch from 16d591c to 89ade9f Compare September 25, 2026 00:30
@is-goutham

Copy link
Copy Markdown
Contributor Author

GitHub stack rebase completed after #302 merged. This branch was rebased linearly onto the updated branch below it and force-pushed with lease. Final rebased #309 validation: 960 relevant tests passed; the 3 excluded router assertions reproduce unchanged on current main; PowerShell parser passed.

Base automatically changed from users/gouthams/pr290-04-workday-da-discovery to main September 25, 2026 00:32
@is-goutham
Surendra Goutham (is-goutham) force-pushed the users/gouthams/pr290-05-workday-installer branch from 89ade9f to 1245dc3 Compare September 25, 2026 00:32
@is-goutham
Surendra Goutham (is-goutham) merged commit 94b602c into main Sep 25, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants