ClosedLess/Microsoft-Edge-Updater
ClosedLess/Microsoft-Edge-Updater
2
1
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

program-rename (LTS) #1

Merged
TheOnePath merged 6 commits from program-rename into main 2022-02-20 14:37:21 +00:00
Conversation 1 Commits 6 Files Changed 5 +56 -53
TheOnePath commented 2022-02-15 22:50:20 +00:00
Owner
Copy Link

A pull request to merge the project rename into the main branch.

This PR will be the official closing future releases for this version of the software, marking v0.2.1 as the LTS. With this, will begin a new redevelopment of the software that is to come in the future; marking the beginning of v1.0.0.

Since this will become the LTS, any critical bugs will be addressed; however, no new features are to be added to v0.2.1, and future features will be implemented in v1.*.

:@TheOnePath:
This project was amazing to develop on, but is bound to a language with limited functionality, meaning much more can be done in another language.

You served us well updater, but the end is the beginning; forth to renaissance!

A pull request to merge the project rename into the main branch. This PR will be the official closing future releases for this version of the software, marking v0.2.1 as the LTS. With this, will begin a new redevelopment of the software that is to come in the future; marking the beginning of v1.0.0. Since this will become the LTS, any critical bugs will be addressed; however, no new features are to be added to v0.2.1, and future features will be implemented in v1.*. :@TheOnePath: This project was amazing to develop on, but is bound to a language with limited functionality, meaning much more can be done in another language. You served us well updater, but the end is the beginning; forth to renaissance!
TheOnePath added 5 commits 2022-02-15 22:50:21 +00:00 
Renamed the program from 'updater' to 'msedge-updater'.
Updated all files related to the original name to adopt new name.
Since file was renamed to `msedge-updater`, check if Edge process is
running with simply "msedge" is no longer suitable.
    - changed this to "msedge-$edge-channel"
Updated the version to 0.2.1 for minor changes
Updated contact information for Ethan.
TheOnePath requested review from robert 2022-02-15 22:50:31 +00:00
TheOnePath removed review request for robert 2022-02-15 22:50:34 +00:00
TheOnePath requested review from Admins 2022-02-15 22:50:38 +00:00
TheOnePath added the
LTS
enhancement
 labels 2022-02-15 22:53:23 +00:00
TheOnePath added this to the LTS milestone 2022-02-17 18:33:57 +00:00
robert reviewed 2022-02-17 20:30:25 +00:00
msedge-updater Outdated
@ -0,0 +302,4 @@
[[ ! $quiet -eq 0 ]] && >&2 echo "Identified a new release of Microsoft Edge ($edge_channel) [Current: v$current_version. New: v$release_version]. Starting the download and installation process..."
## check if msedge process is already running
is_running=$(ps -aux | grep -oc "msedge-$edge_channel")
robert commented 2022-02-17 20:30:25 +00:00
Owner
Copy Link

This check works, However I can see a false positive scenario occuring.
If a long running command contains msedge-$edge_channel then it will be included in the ouput of the check.

For example

watch pidof msedge-dev &
ps -aux | grep -oc "msedge-dev" # assume dev channel

Would give an output of 1 Even though msedge is not actually running.

I would suggest the use of a longer and slightly more verbose check.

pidof msedge | xargs ps -ocmd -p | grep -m1 "msedge-$edge_channel"

In this check we:

  • Use pidof msedge to return the PID(s) of any/all running msedge process(es)
  • Pipe the PID(s) to xargs, which uses ps to get the cmdline
    • We need xargs to position the PID(s)
    • The ps flag -ocmddefines the output as just the cmdline
    • The ps flag -p tells ps to just look for the given PID(s)
  • Grep for a maximum of one match for the given edge channel

It's also uneccesary to use the count flag in grep since no matches returns an empty result which is easy to test for.
EDIT: fix minortypo

This check works, However I can see a false positive scenario occuring. If a long running command contains `msedge-$edge_channel` then it will be included in the ouput of the check. For example ```bash watch pidof msedge-dev & ps -aux | grep -oc "msedge-dev" # assume dev channel ``` Would give an output of 1 Even though msedge is not actually running. I would suggest the use of a longer and slightly more verbose check. ```bash pidof msedge | xargs ps -ocmd -p | grep -m1 "msedge-$edge_channel" ``` In this check we: - Use `pidof msedge` to return the PID(s) of any/all running msedge process(es) - Pipe the PID(s) to xargs, which uses ps to get the cmdline - We need xargs to position the PID(s) - The ps flag `-ocmd`defines the output as just the cmdline - The ps flag `-p` tells ps to just look for the given PID(s) - Grep for a maximum of one match for the given edge channel It's also uneccesary to use the count flag in grep since no matches returns an empty result which is easy to test for. EDIT: fix minortypo
robert requested changes 2022-02-17 20:37:11 +00:00
robert left a comment
Owner
Copy Link

Overall looks good,
However I am requesting changes due to an edge case issue that I noticed with the is_running check.

I have made a comment on the particular line with my thoughts and recommendations.
If you disagree I am happy to discuss further in the comments.

Overall looks good, However I am requesting changes due to an edge case issue that I noticed with the is_running check. I have made a comment on the particular line with my thoughts and recommendations. If you disagree I am happy to discuss further in the comments.
TheOnePath commented 2022-02-17 23:12:00 +00:00
Author
Owner
Copy Link

I would not have considered something like this to be part of the current PR given its purpose; however, a commit was made to fix a bug, which works but as outlined in the review above, would lead to unsuitable edge cases.

Removing the commit for the bug fix (9ff1b6e40e) would likely be more danergous than having it in. Even though this commit is purposefully for renaming the software, it is also the start of the LTS, so addressing bugs is always essentially.

I'm content with the implementation on top of the bug fix since a solution was provided in detail and clarity during the review. This is an exception however, and future bugs MUST be raised as an Issue.

I would not have considered something like this to be part of the current PR given its purpose; however, a commit was made to fix a bug, which works but as outlined in the review above, would lead to unsuitable edge cases. Removing the commit for the bug fix (https://git.closedless.xyz/ClosedLess/Microsoft-Edge-Updater/commit/9ff1b6e40e197e6dd6b2d134d7956876e5bd0cce) would likely be more danergous than having it in. Even though this commit is purposefully for renaming the software, it is also the start of the LTS, so addressing bugs is always essentially. I'm content with the implementation on top of the bug fix since a solution was provided in detail and clarity during the review. This is an exception however, and future bugs **MUST** be raised as an Issue.
TheOnePath added 1 commit 2022-02-17 23:32:18 +00:00 
The bug fix in commit 9ff1b6e has been re-addressed due to a potential
edge case to cause another bug to occur as a result. This fix was
implemented as a result from the review on PR: #1 (comment)
TheOnePath requested review from robert 2022-02-17 23:32:36 +00:00
robert approved these changes 2022-02-20 01:47:19 +00:00
robert left a comment
Owner
Copy Link

Having cleared up the minor issue I can see no further problems with this PR
Feel free to merge whenever you're ready

Having cleared up the minor issue I can see no further problems with this PR Feel free to merge whenever you're ready
TheOnePath changed title from [WIP] program-rename (LTS) to program-rename (LTS) 2022-02-20 13:13:31 +00:00
TheOnePath merged commit 9a3c516fe3 into main 2022-02-20 14:37:21 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
robert
Labels
Clear labels
  
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
LTS

Mark issue or PR as an LTS

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
bug
duplicate
enhancement
help wanted
invalid
LTS
question
wontfix
Milestone
Clear milestone
No items
No Milestone
LTS
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: ClosedLess/Microsoft-Edge-Updater#1
No description provided.
Delete Branch "program-rename"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?