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

Fix path to required version data #969

Merged

Conversation

crutchfield
Copy link
Contributor

@crutchfield crutchfield commented Mar 6, 2024

🗣 Description

Identify module base dir and get RequirementVersions.ps1 relative to that instead of relying on $PSScriptRoot
closes #968

💭 Motivation and context

Initialize-SCuBA was failing to find RequiredVersions.ps1 when executed from a owrkflow running.

🧪 Testing

  • Ran Invoke-SCuBA from repo root
  • Ran Invoke-SCuBA from home directory
  • Ran Invoke-SCuBA from "Nightly Product Test" workflow on a runner.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • PR targets the correct parent branch (e.g., main or release-name) for merge.
  • Changes are limited to a single goal - eschew scope creep!
  • Changes are sized such that they do not touch excessive number of files.
  • All future TODOs are captured in issues, which are referenced in code comments.
  • These code changes follow the ScubaGear content style guide.
  • Related issues these changes resolve are linked preferably via closing keywords.
  • All relevant type-of-change labels added.
  • All relevant project fields are set.
  • All relevant repo and/or project documentation updated to reflect these changes.
  • Unit tests added/updated to cover PowerShell and Rego changes.
  • Functional tests added/updated to cover PowerShell and Rego changes.
  • All relevant functional tests passed.
  • All automated checks (e.g., linting, static analysis, unit/smoke tests) passed.

✅ Pre-merge checklist

  • PR passed smoke test check.

  • Feature branch has been rebased against changes from parent branch, as needed

    Use Rebase branch button below or use this reference to rebase from the command line.

  • Resolved all merge conflicts on branch

  • Notified merge coordinator that PR is ready for merge via comment mention

✅ Post-merge checklist

  • Feature branch deleted after merge to clean up repository.
  • Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

@crutchfield crutchfield added the bug This issue or pull request addresses broken functionality label Mar 6, 2024
@crutchfield crutchfield added this to the Glacier milestone Mar 6, 2024
@crutchfield crutchfield self-assigned this Mar 6, 2024
@crutchfield crutchfield linked an issue Mar 6, 2024 that may be closed by this pull request
Copy link
Collaborator

@schrolla schrolla left a comment

Choose a reason for hiding this comment

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

Tested fix by running Initialize-SCuBA from the checkout repo directory, from its grandparent directory, and reviewing the nightly testing action taken previously. Also ran Invoke-SCuBA to ensure it ran as expected and it did.
Code review shows update to ModuleBaseDir as a solid location from which to base calculation of the RequiredVersions.ps1 relative script location. See requested change below for minor updates for it to function properly in workflow scenarios.

@james-garriss
Copy link
Collaborator

Please add the catch while you're there, something like this:

    if (Test-Path -Path $RequiredModulesPath) {
        . $RequiredModulesPath
    }
    else {
        Write-Error "Unable to find required modules path at $RequiredModulesPath"
    }

@crutchfield crutchfield force-pushed the 968-initialize-scuba-is-start-location-dependent branch from 7a2a513 to a410e52 Compare March 7, 2024 11:56
Copy link
Collaborator

@james-garriss james-garriss left a comment

Choose a reason for hiding this comment

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

Thanks for good work.

@james-garriss james-garriss mentioned this pull request Mar 7, 2024
14 tasks
Copy link
Collaborator

@schrolla schrolla left a comment

Choose a reason for hiding this comment

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

Updated code works when tested in both the expected and error condition cases. Ran Invoke-Scuba successfully on test tenant and ran Initialize-SCuBA successfully. Renamed RequiredVersions.ps1 and ran again and received expected error condition. Looks good to me.

@crutchfield
Copy link
Contributor Author

@nanda-katikaneni PR is ready to merge to main. Smoke test failure unrelated to PR.

@nanda-katikaneni nanda-katikaneni merged commit 7f03cb1 into main Mar 7, 2024
14 of 18 checks passed
@nanda-katikaneni nanda-katikaneni deleted the 968-initialize-scuba-is-start-location-dependent branch March 7, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialize-SCuBA is start location dependent.
4 participants