Skip to content

Update logic improved to work with slightly renamed script files#1660

Merged
lusassl-msft merged 3 commits into
mainfrom
lusassl-ScriptUpdateImprovements
Apr 14, 2023
Merged

Update logic improved to work with slightly renamed script files#1660
lusassl-msft merged 3 commits into
mainfrom
lusassl-ScriptUpdateImprovements

Conversation

@lusassl-msft
Copy link
Copy Markdown
Contributor

Description:
In the past, the script update logic failed if the script name wasn't exactly the same as the original file name (e.g., due to multiple files within a folder, the script that was executed is HealthChecker (1).ps1).

The improved logic will work, even if the script was slightly renamed. However, it wouldn't work if the script were renamed from e.g., HealthChecker.ps1 to HC.ps1.
There are also ways to make this combination work, however, this requires some general adjustments to the build process and function if we want to make this work.

@lusassl-msft lusassl-msft added Enhancement New feature or request Shared Function labels Apr 14, 2023
@lusassl-msft lusassl-msft requested a review from a team as a code owner April 14, 2023 10:03
@lusassl-msft lusassl-msft changed the title Update logic improved to work with renamed script files Update logic improved to work with slightly renamed script files Apr 14, 2023
@dpaulson45
Copy link
Copy Markdown
Member

Not sure if we should be taking into account these scenarios. How often does this occur in a customer's environment where they are manually downloading the script and where the update feature does work?

@lusassl-msft
Copy link
Copy Markdown
Contributor Author

lusassl-msft commented Apr 14, 2023

I'm not sure if this is a common issue. I experienced it myself and got one report from a customer. In my case the script was downloaded and renamed as it was already in the download folder. In the customer reported case, the script passed a security check and was renamed to CVE-2023-23397_clean.ps1 before it was executed for the first time and so, never got an update via the auto-update function.

@dpaulson45
Copy link
Copy Markdown
Member

If it passes a security check and they renamed the file. I am sure the security team would like it to not be automatically updated to bypass a security review.

@lusassl-msft
Copy link
Copy Markdown
Contributor Author

Yeah, don't ask if that makes sense. However, they uncovered this behavior and reported it.

@bill-long
Copy link
Copy Markdown
Member

If we're going to do this, I kind of like the idea of coding the original script name into the script (via a build process variable, similar to $BuildVersion). But I'm still on the fence as to whether we should do this at all.

Another approach would be to write out a warning when we check for updates and there's no such file name in the repo. Then at least the user knows it's not updating due to the name.

@dpaulson45
Copy link
Copy Markdown
Member

If we're going to do this, I kind of like the idea of coding the original script name into the script (via a build process variable, similar to $BuildVersion). But I'm still on the fence as to whether we should do this at all.

Another approach would be to write out a warning when we check for updates and there's no such file name in the repo. Then at least the user knows it's not updating due to the name.

I kind of like that approach better that it is not updating due to the name. There is a lot more logic to add to handle it the other way as well. It is better to just K.I.S.S. it.

Keep It Simple Stupid

@lusassl-msft
Copy link
Copy Markdown
Contributor Author

@bill-long
If we're going to do this, I kind of like the idea of coding the original script name into the script (via a build process variable, similar to $BuildVersion). But I'm still on the fence as to whether we should do this at all.

Exactly, that's what I mean when I said that we must adjust the build process if we really want to do that.

Another approach would be to write out a warning when we check for updates and there's no such file name in the repo. Then at least the user knows it's not updating due to the name.

Reads also good to me and would at least let the user know that action is required on their side to benefit from the latest improvements.

@dpaulson45
Copy link
Copy Markdown
Member

@bill-long If we're going to do this, I kind of like the idea of coding the original script name into the script (via a build process variable, similar to $BuildVersion). But I'm still on the fence as to whether we should do this at all.

Exactly, that's what I mean when I said that we must adjust the build process if we really want to do that.

Another approach would be to write out a warning when we check for updates and there's no such file name in the repo. Then at least the user knows it's not updating due to the name.

Reads also good to me and would at least let the user know that action is required on their side to benefit from the latest improvements.

If we can go out to the repo and there is not script with this name, then we should fail out. There should be no reason to continue unless this is deliberate by the end user. If it is on purpose they want to not update, then the end user can provide to skip the update or correctly change the name.

@lusassl-msft
Copy link
Copy Markdown
Contributor Author

Okay, if we all agree that we want to go with the "showing a warning" approach instead of trying to find the matching script, I'll make the adjustments and add another commit.

@dpaulson45
Copy link
Copy Markdown
Member

dpaulson45 commented Apr 14, 2023

Is this how we want the function to work? Just throw the warning and continue with the script?

@lusassl-msft
Copy link
Copy Markdown
Contributor Author

lusassl-msft commented Apr 14, 2023

Is this how we want the function to work? Just throw the warning and continue with the script?

I would say yes, we should not prevent customers from running the script in case that auto-update fails due to a script that was renamed or so.

@bill-long
Copy link
Copy Markdown
Member

Agreed. Just warn and continue. We should't fail out due to an update check failure.

@lusassl-msft lusassl-msft merged commit 298cced into main Apr 14, 2023
@lusassl-msft lusassl-msft deleted the lusassl-ScriptUpdateImprovements branch April 14, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement New feature or request Shared Function

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants