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

windows compatibility #226

Merged
merged 7 commits into from
Jan 26, 2023
Merged

windows compatibility #226

merged 7 commits into from
Jan 26, 2023

Conversation

lordvlad
Copy link
Collaborator

Chown does not exist on widows. This PR uses nodejs's native fs.chmodSync instead

@lordvlad lordvlad changed the title fix(build): remove dependency on *nix chown WIP fix(build): remove dependency on *nix chown Jan 12, 2023
@lordvlad lordvlad marked this pull request as draft January 12, 2023 21:57
@lordvlad lordvlad changed the title WIP fix(build): remove dependency on *nix chown fix(build): remove dependency on *nix chown Jan 12, 2023
@lordvlad lordvlad marked this pull request as ready for review January 14, 2023 12:15
@lordvlad lordvlad marked this pull request as draft January 14, 2023 13:01
@lordvlad lordvlad changed the title fix(build): remove dependency on *nix chown windows compatibility Jan 16, 2023
@lordvlad lordvlad marked this pull request as ready for review January 26, 2023 21:22
* @param dir target directory
* @returns promise for the full path of the downloaded file
*/
async function download(url: string, dir: string): Promise<string> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might seem unorthodox to implement this from scratch, but really, introducing another library was not worth it IMHO

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, mutch apreciated!

src/bin/tools/downloadAndUnzip.ts Outdated Show resolved Hide resolved
pathOfDirToExtractInArchive?: string;
cacheDirPath: string;
}) {
const downloadHash = hash(JSON.stringify({ url, pathOfDirToExtractInArchive })).substring(0, 15);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now a bit different to your previous cachin strategy, let me know what you think of it

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM in surface.
I didn't test if it actually work though...
I'll trust you on that.

var { bin } = await import(pathJoin(getProjectRoot(), "package.json"));

var promises = Object.values<string>(bin).map(async scriptPath => {
const fullPath = pathJoin(getProjectRoot(), scriptPath);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a bit more synchronous, eh? :)

@garronej
Copy link
Collaborator

Thank you so much for this work.
I'm sure the community will greatly appreciate it!

@garronej garronej merged commit 564e142 into keycloakify:main Jan 26, 2023
@garronej garronej mentioned this pull request Jan 27, 2023
@garronej
Copy link
Collaborator

Bummer, it dosen't work.
@lordvlad, can you pick it up from here
#233 ?

here is an invite on for the repo so you can push on the branch: https://github.com/InseeFrLab/keycloakify/invitations

@lordvlad
Copy link
Collaborator Author

@garronej whats broken? it worked for me and github actions were all green?

@garronej
Copy link
Collaborator

@lordvlad I'm very very sorry 🙇🏼‍♂️
It's working fine, I just ran the wrong command.

@garronej
Copy link
Collaborator

Again @lordvlad,
I wanted to reiterate and tell you how very sorry I am to call your PR broken when it was I that didn’t run the right command.
Your effort is very much appreciated!

@lordvlad
Copy link
Collaborator Author

lordvlad commented Feb 2, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants