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

Update Powershell module requirements #1368

Merged
merged 11 commits into from
Feb 2, 2023

Conversation

craddm
Copy link
Contributor

@craddm craddm commented Jan 31, 2023

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the develop branch.
  • Your branch is up-to-date with the develop branch (you probably started your branch from develop but it may have changed since then).
  • If-and-only-if your changes are not yet ready to merge, you have marked this pull request as a draft pull request and added '[WIP]' to the title.
  • If-and-only-if you have changed any Powershell code, you have run the code formatter. You can do this with ./tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory>.

⤴️ Summary

Updates several required Powershell module versions to be compatible with more recent versions (7.3.x) of Powershell (see #1332). The updated modules are also still compatible with several Powershell versions after testing (7.2.8, 7.2.9).

CheckRequirements.ps1 now warns the user if they are using an unsupported Powershell version, with the supported version being the latest stable version (currently 7.3.2). Also fixes retrieval of Powershell version numbers, which were incorrectly retrieved in some circumstances.

Adds an additional module to CheckRequirements.ps1 - Microsoft.Graph.Users. Some functions from this module are used when deploying an SHM.

Updates the .devcontainer to use Powershell 7.3.2.

Updates the documents to suggest users use the supported Powershell version.

🌂 Related issues

Closes #1361
Closes #1369
Closes #1373

🔬 Tests

Ran several (SRE and SHM) deployment steps that were failing on Powershell versions 7.3.x with updated module versions to confirm that the updated versions fixed the issues (see #1332). Also ran the same steps on versions 7.2.6, 7.2.8, and 7.2.9 to check that the updated modules were compatible.

@craddm craddm marked this pull request as draft January 31, 2023 11:58
@craddm
Copy link
Contributor Author

craddm commented Jan 31, 2023

May have discovered another related issue, switching to draft while I double-check

@JimMadge
Copy link
Member

When it comes to the powershell version, I think we should try to only support one of

  • The latest stable (currently 7.3.2)
  • The latest LTS (currently 7.2.9)

It really doesn't make sense to support unmaintained powershell versions and the more we try to support the more difficult it will be to find a set of working module versions.

@craddm
Copy link
Contributor Author

craddm commented Jan 31, 2023

When it comes to the powershell version, I think we should try to only support one of

  • The latest stable (currently 7.3.2)
  • The latest LTS (currently 7.2.9)

It really doesn't make sense to support unmaintained powershell versions and the more we try to support the more difficult it will be to find a set of working module versions.

Ok, so in practical terms does this mean increasing the minimum Powershell version in CheckRequirements.ps1 to 7.2.9, and/or changing the documentation to reflect our intention to only support the latest stable/latest LTS versions, rather than only stipulating v7.0 or above? Or maybe adding a warning message if an "unsupported" version of Powershell is used?

@jemrobinson
Copy link
Member

It's worth saying that we only support latest stable and latest LTS without directly enforcing in the code. It does look like we could check for >=7.2.9 in the code though?

@JimMadge
Copy link
Member

We should be clear in the documentation.

Having a warning in CheckRequirements.ps1 should be helpful. I think a warning rather than a hard stop is better.

It's worth saying that we only support latest stable and latest LTS

I get the impression that trying to support both is causing problems, particularly with the large number of modules. I think it might be better to choose just one.

@craddm
Copy link
Contributor Author

craddm commented Jan 31, 2023

Ok, so will add an additional check in CheckRequirements.ps1 that will simply warn, rather than error, and update the docs.

As far as I can tell the modules that are compatible with 7.3.0+ are also compatible with 7.2.9 etc., just not the other way round. But it'll definitely be easier to commit to a specific supported version.

@craddm craddm marked this pull request as ready for review February 1, 2023 16:26
@jemrobinson
Copy link
Member

Maybe we can/should drop the whole PowershellMinVersion and PowershellSupportedVersion checks and just say "we recommend you use the latest stable Powershell version - we've most recently tested this using version X"?

Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@craddm craddm merged commit 7c2d29f into alan-turing-institute:develop Feb 2, 2023
@craddm craddm deleted the update-requirements branch February 2, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants