Features & Refactors #64
Conversation
src/utils/commands.ts
Outdated
| tempDIR, | ||
| baseURL, | ||
| } from "./globals"; | ||
| import { destDir, user, tempDIR, baseURL, platform, root } from "./globals"; | ||
|
|
||
| export const codespacesCommands = ( |
There was a problem hiding this comment.
What is the purpose of leaving the codespacesCommands in here 🤔 I see you have generalised the windows/mac/Linux commands, what is the purpose of not doing the codespaces either 🤔 is it because we call this function specifically if it's codespaces.
I see quite a few of the command objects are reused across the codespacesCommands and generalCommands. If we are going down your approach (which I like by the way) it may be worth exporting each command as it's own object and then referencing the commands, it would save us copying the same command syntax for codespaces and general, if that makes sense?
E.G
export const gitCloneCommand = { ... }
---
import { gitCloneCommand } from "./"
Does that make sense? I love the idea of going general, but I think we can go a step further and do the same for codespaces. Maybe create a directory called commands and then have a single file in each directory for each command. Import all of them commands into the index.ts in the commands directory, then just import the commands you need into the src/utils/commands.ts file 👍
Let me know if that makes sense 👍
There was a problem hiding this comment.
Absolutely agree, codespace could be consolidated.
It's been a minute since I've looked at it and struggling to remember why I didn't include it. I thought I put it in the commit log, but I don't see it.
At a glance, I think it had to do with the cwd being different. There's no sense of a user path, which made it unique. Now that I think about it root could just include user if it's linux, MacOS or windows. Also, I'm not sure how it works but do you get an entirely new instance each time you use it? Do you even need to rm any files/folders?
In the end I think it was a time constraint and that I've never worked with codespace.
Not sure I follow on Maybe create a directory called commands and then have a single file in each directory for each command. Do you mean each and every command or based on git vs OS commands?
I'm a bit of a map lover and would maybe mix the two. Something like below. I'm more proficient in groovy so my example will be in that and without writing out the entire thing...
branch = 'ghas-test'
platform = 'darwin'
osPlatform = (platform == 'win32') ? platform : 'nix'
cwd = 'Users/djthornton1212/Desktop/test'
osCommands = [
mkdir : [
win32: 'mkdir',
nix: 'mkdir -p'
],
rm : [
win32: 'rmdir /Q /S',
nix: 'rm -rf'
],
copy: [
win32: 'copy',
nix: 'cp'
]
]
gitCommands = [
clone: [
general: 'git clone',
darwin: 'git clone --depth 1 --filter=blob:none --sparse'
],
checkout: [
general: "git checkout -b ${branch}"
],
add: [
general: 'git add'
],
commit: [
general: 'commit -m "Commit CodeQL File"'
],
push: [
general: "git push --set-upstream origin ${branch}"
]
]
ghasCommands = [
(osCommands.rm."$osPlatform" + " $cwd"),
(osCommands.mkdir."$osPlatform" + " $cwd"),
(platform == 'darwin' ? gitCommands.clone.darwin : gitCommands.clone.general),
gitCommands.checkout.general,
gitCommands.add.general,
gitCommands.commit.general,
gitCommands.push.general,
(osCommands.rm."$osPlatform" + " $cwd"),
]
for (command in ghasCommands ) {
println "executing: $command"
}
This way we just export commandOrder... I don't know need to think that over some.
There was a problem hiding this comment.
Okay, let's focus on this:
Absolutely agree, codespace could be consolidated.
don't worry about the second part, I think it's fine as it is 🙇 But I think it would be good to consolidate down the commands to Codespaces as well 👍
Also, I'm not sure how it works but do you get an entirely new instance each time you use it? Do you even need to rm any files/folders?
Great question, each codespace is a fresh instance, meaning for everyone you create, you get new folders, etc. However, the codespace can last for however long you need to. You can reuse the same codespace, but most people just start from scratch.
It would be super easy to test, just create a new org (it's free) and then create three repos:
- The repository where you will run the script from (this is where you will create the codespace from)
- A Javascript repo with a single file in called
hello.js - Another Javascript repo with a single file in called
hello.js
Then just follow the steps here and run the script. It should be super simple, Codespaces are nice and quick to get up and running.
That would allow you to test your changes quick and easily :)
If you could do this, then I think it would make this change 💯
There was a problem hiding this comment.
@NickLiffen Quick update, though organizations are free, codespaces aren't.
Codespaces is available for organizations using GitHub Team or GitHub Enterprise Cloud. For more information,
I'm discussing with my management about using it in our organization, but at the moment don't have it available to me. Can you test that component once I update the PR?
There was a problem hiding this comment.
Yes, of course, I think you would like Codespaces if you can use it, but if you need me to test, I am happy to for sure 🙇
src/utils/commitFile.ts
Outdated
| "We detected an OS that wasn't Windows, Linux or Mac. Right now, these " + | ||
| "are the only three OS's supported. Log an issue on the repository for " + | ||
| "wider support" |
There was a problem hiding this comment.
| "We detected an OS that wasn't Windows, Linux or Mac. Right now, these " + | |
| "are the only three OS's supported. Log an issue on the repository for " + | |
| "wider support" | |
| `We detected an OS that wasn't Windows, Linux or Mac. Right now, these are the only three OS's supported. Log an issue on the repository for wider support` |
The use of backticks here should be able to mean you shouldn't need to use the + symbol 👍
There was a problem hiding this comment.
Good to know. Still learning in this language. I think I was trying to not pass the 80 character line or something.
There was a problem hiding this comment.
For sure 🙇 if you get this changed that would be amazing 🙇
src/utils/globals.ts
Outdated
| export const reposFileLocation = "./bin/repos.json" as string; | ||
| export const orgsFileLocation = "./bin/organizations.json" as string; | ||
| export const platform = os.platform() as string; | ||
| export const destDir = | ||
| platform === "win32" ? ("Documents" as string) : ("Desktop" as string); |
There was a problem hiding this comment.
Does Desktop not exist on Windows? I always wanted to keep this the same across OS's.
Is there a reason why for Windows we are using Documents 🤔
There was a problem hiding this comment.
It does, I was just keeping with what you already had. Didn't know if you have reason to use Documents to begin with. It can/should be consolidated... Desktop would work just fine imho.
There was a problem hiding this comment.
If you could move it to Desktop that would be amazing 🙇
|
@djthornton1212 AMAZING work 🙇 100% would love to get this merged in. It looks brilliant work 🙇 I listed some thoughts above, keen to get your thoughts on these. I would really love to get these changes proposed into this project, so let me know your thoughts 🙇 |
|
Okay @djthornton1212 I see some small changes:
Then we can go ahead and get this merged in 🙇 Thank you for your contributions 🙇 |
|
@djthornton1212 👋 Is this still something you are able to work on 🙇 if not no worries, I know things change. I would be happy to take this up, but I know you have spent a lot of time working on this so just want to double-check 👍 |
|
Hey @NickLiffen I do, it's just been a bit busy with work. I'll likely have to do this during my weekend. |
No worries 🙇 please take your time, I just wanted to check in 🙇 thank you so much for your work here 🙇 i will leave this to your time. |
This change is to allow repos.json & organizations.json to be added to gitignore so that privacy can be maintained in private organizations.
Not sure if this was intentional, but a java codeql file doesn't exists. This commit introduces the file
Update documentation. `Yarn add` expects a package. The equivalent of npm install (install all deps)ya is `yarn install` or just `yarn` See: https://classic.yarnpkg.com/lang/en/docs/cli/install/
This commit attempts to simplify the determination of the destination directory and username. It also introduces the root for user home to be used in later feature.
This commit is largely a refactor of the existing code. The point is to
simplify the commands required for windows/macos/linux. They largely
all use the same commands with slight variations, such as home
directory, and command arguments.
It is a larger commit than I'd like, but all three file changes do go
together.
globals.ts:
- Removes the need to create separate vars for windows vs
everything else. Determine the differences in globals and send 1 var.
- Determine OS here so we can set root to home path. No longer needed
in commitFile.ts
commands.ts:
- Squash windows/macOS/linux into a single command array.
- Introduce platform and root to differ commands/paths
- Remove unnecessary commands like mv as cp can accomplish this
- Make commands/arguments conditional
- Add helper function to convert linux paths to windows paths
- This really helps with mkdir as Windows can natively create
multi-leveled directory structures when correct paths are used: `\`
commitFile.ts:
- Moved existsSync, os and isCodespace to globals.ts
- Replaced mac/windowsCommands w/ genearlCommands
- Simplified gitCommands condition
- Added informs
- Added try catch to "for loop" of commands. I may just be missing it,
but the If (stderr) block didn't seem to be activating. The app would
die after stating the error.
- Whitelist function for known error responses like this folder alredy
exists and can't delete a folder that doesn't exists.
The whitelist function feels hacky, so I welcome any thoughts on it.
|
@NickLiffen Ok, I've started updating the PR. I've resolved the conflict. That was a mess. I'll run some tests and see how it looks. About the workflow checks, the prettier check. What should I do for that? |
Run
Thank you so much for your hard work on this 🙇 making the lives better for others is going to be so useful 🙇 |
Yeah, I ran that and it touched almost every file but I couldn't see the difference in the files. Also, I still need to merge the requested changes into my main then we'll be ready. I'll keep you apprised. |
This commit contains the recommended changes from Nick. 1. globals consolidates all OS types to a common cwd under destdir 2. The commands.ts file consolidates all commands into a single list 3. Backticks are used in string block. Also introduces user specified temp directory. It turns out I don't have a ~/Desktop thanks to OneDrive. ISSUE-61
feat: ✨ ISSUE-61 PR comments
|
@NickLiffen Ok, the requested changes are in. I also added a temp directory option.
It allows the user to choose a different path. |
|
@djthornton1212 👋 IT WORKS!!!! 👯 YAY!! I just tested it on Codespaces and it works perfectly! nice work 🙇 |
🍾That's great news! 🍾 I can confirm it also works on Windows 10 & Windows 10 WSL Ubuntu. That's as close to Linux as I have. I don't have a mac to test it on though... I miss my mac. 😢 |
|
@NickLiffen Ok, so do you want to run the checks? It says they are pending. Once those are done am I merging or are you? It says it's assigned to me, so I assume me. |
|
@djthornton1212 no worries 👍 you have run on Windows right and it works? Just for the sake of testing, could you post the outcome of running the |
|
I just tested this on Mac and Windows myself (I spun up a VM) and it worked 🙇 I am going to merge this PR. @djthornton1212 THANK YOU for your work 🙇 I am going to give you write access to this repository from now on, and welcome your contributions always 🙇 |
|
@NickLiffen Sorry for the late response. Had to do work stuff. Great! Thank you for creating this. It saved me a lot of manual work. I'm thinking about using components for a batch workflow updater. I have to manage around 100 repos and trying to touch each manually can be tedious. |
Pull Request Template
FEAT|REFACTOR:
GitHub Issue #61
Description
There are several changes introduced in this pull request. Note that each commit has details listing the changes in them. These will be summarized here.
In an effort to make this functional for our environment we focused around refactoring the commands.ts file so that it is a bit more universal. By combining mac/linux/windows commands into a single array set we can ensure that each OS type is receiving the same set of instructions though the commands/arguments themselves may differ slightly.
Changes
This refactors:
globals.ts:
commands.ts:
commitFile.ts:
The whitelist function feels hacky, so I welcome any thoughts on it.
This adds:
globals.ts:
codeql-analysis-java.yml:
commands.ts:
/->\\commitFile.ts:
This removes:
Testing