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

feat: Make git-proxy executable, custom config file arg & validation, bump to 1.1.0 #287

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

coopernetes
Copy link
Contributor

@coopernetes coopernetes commented Sep 16, 2023

  • add "bin" to package.json to support running git-proxy as an
    executable. Largely taken from Support npx usage and fix default configuration loading #184 and fixes Make installation and usage of the library easier #183
  • added --config and --validate command line flags via yargs. These will
    allow custom user configurations with a default behaviour of loading
    from the current working dir and/or default settings. --validate uses
    JSON Schema to validate the config.
  • update docs to reflect new CLI args and additional details of running
    the app via npx
  • load default settings from a module instead of explicit file path
  • add test for default & user setting merging
  • bump @finos/git-proxy to 1.1.0
  • Remove old X.509 certificate and private key, which I assume was
    included previously to run git-proxy with TLS enabled via a demo. Can
    be re-added as needed but probably shouldn't be included in src (even
    if its for demo only).

@coopernetes
Copy link
Contributor Author

coopernetes commented Sep 16, 2023

New executable (npm link simulates npm install -g):

$ docker run -it -v $(pwd):/src node:18-slim bash
root@577185b94f6b:/$ cd /src
root@577185b94f6b:/src$ npm link

added 1 package, and audited 3 packages in 1s

found 0 vulnerabilities
root@577185b94f6b:/src$ cd ~
root@577185b94f6b:~$ which git-proxy
/usr/local/bin/git-proxy
root@577185b94f6b:~$ ls -l /usr/local/bin/
total 89996
lrwxrwxrwx 1 root root       45 Aug  8 22:27 corepack -> ../lib/node_modules/corepack/dist/corepack.js
-rwxrwxr-x 1 root root      388 Aug 16 02:01 docker-entrypoint.sh
lrwxrwxrwx 1 root root       45 Sep 16 23:05 git-proxy -> ../lib/node_modules/@finos/git-proxy/index.js
-rwxr-xr-x 1 root root 92150296 Aug  8 22:27 node
lrwxrwxrwx 1 root root       19 Aug 16 02:02 nodejs -> /usr/local/bin/node
lrwxrwxrwx 1 root root       38 Aug  8 22:27 npm -> ../lib/node_modules/npm/bin/npm-cli.js
lrwxrwxrwx 1 root root       38 Aug  8 22:27 npx -> ../lib/node_modules/npm/bin/npx-cli.js
lrwxrwxrwx 1 root root       27 Aug 16 02:02 yarn -> /opt/yarn-v1.22.19/bin/yarn
lrwxrwxrwx 1 root root       30 Aug 16 02:02 yarnpkg -> /opt/yarn-v1.22.19/bin/yarnpkg
root@577185b94f6b:~$ git-proxy 
Listening on 8000
creating user
        user=admin,
        gitAccount=none
        email=admin@place.com,
        admin=true
Service Listening on 8080
^C

Create a proxy.config.json file in the local directory. It will now be read by git-proxy on startup and merged with the default settings.

root@577185b94f6b:~# cat << EOF > proxy.config.json
> { "authentication": [ { "type": "foo", "enabled": true } ] }
> EOF
# this is an intentionally dummy config
root@577185b94f6b:~$ git-proxy
/src/src/service/passport/index.js:18
      throw Error(`uknown authentication type ${type}`);
            ^

Error: uknown authentication type foo
    at Object.configure (/src/src/service/passport/index.js:18:13)
    at Object.start (/src/src/service/index.js:20:48)
    at Object.<anonymous> (/src/index.js:6:9)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47

Node.js v18.17.1

@coopernetes coopernetes marked this pull request as draft September 16, 2023 23:49
@coopernetes
Copy link
Contributor Author

Added a lightweight "CLI" with --config [file] for specifying custom user config (with a default of $(pwd)/proxy.config.json) and --validate to optionally check a file has the correct values set.

On a fresh Docker container, I'm getting some odd behaviour at startup when using a custom file. It seems to clear after a restart but want to fix this.

$ cat mygitproxysettings.json 
{ "authorisedList": [ { "project": "foo", "name": "bar", "url": "https://github.com/foo/bar.git" } ] }
$ git-proxy --config ./mygitproxysettings.json
Listening on 8000
creating user
        user=admin,
        gitAccount=none
        email=admin@place.com,
        admin=true
Service Listening on 8080
/src/src/db/file/repo.js:58
    if (repo.users.canPush.includes(user)) {
             ^

TypeError: Cannot read properties of null (reading 'users')
    at /src/src/db/file/repo.js:58:14
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v18.17.1

@coopernetes coopernetes changed the title feat: bump to 1.0.1, add exec, merge user config feat: Make git-proxy executable, custom config file arg & validation, bump to 1.0.1 Sep 17, 2023
README.md Outdated
```

To install a specific version of Git Proxy, append the version to the end of the `install` command:

```bash
$ npm install @finos/git-proxy@1.0.0
$ npm install -g @finos/git-proxy@1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

@coopernetes - makes sense to update this to 1.0.1 ?

Copy link
Member

Choose a reason for hiding this comment

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

@maoo @coopernetes - 1.1.0 is probably better as these changes constitute a minor improvement, rather than a patch.

README.md Outdated Show resolved Hide resolved
README.md Outdated
```

To install a specific version of Git Proxy, append the version to the end of the `install` command:

```bash
$ npm install @finos/git-proxy@1.0.0
$ npm install -g @finos/git-proxy@1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Same here with -g?

README.md Outdated Show resolved Hide resolved
@JamieSlome
Copy link
Member

@coopernetes - are you able to take a look at the merge conflicts? Will be from the latest merge of #269.

Thank you 🤝

README.md Outdated
```

To install a specific version of Git Proxy, append the version to the end of the `install` command:

```bash
$ npm install @finos/git-proxy@1.0.0
$ npm install -g @finos/git-proxy@1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$ npm install -g @finos/git-proxy@1.0.0
$ npm install -g @finos/git-proxy@1.1.0

README.md Outdated
```

To install a specific version of Git Proxy, append the version to the end of the `install` command:

```bash
$ npm install @finos/git-proxy@1.0.0
$ npm install -g @finos/git-proxy@1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$ npm install -g @finos/git-proxy@1.0.0
$ npm install -g @finos/git-proxy@1.1.0

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@finos/git-proxy",
"version": "1.0.0",
"version": "1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"version": "1.0.1",
"version": "1.1.0",

@coopernetes @maoo (cc)

README.md Outdated Show resolved Hide resolved
@coopernetes coopernetes changed the title feat: Make git-proxy executable, custom config file arg & validation, bump to 1.0.1 feat: Make git-proxy executable, custom config file arg & validation, bump to 1.1.0 Sep 20, 2023
@coopernetes coopernetes marked this pull request as ready for review September 20, 2023 18:20
@coopernetes
Copy link
Contributor Author

@JamieSlome should be good for review. Let's open a tracking issue re: #287 (comment)

@JamieSlome
Copy link
Member

@coopernetes - I've opened #293 for documentation on configuration options.

I'm a bit confused about how to test this, can you provide the steps I should run in the terminal?

@coopernetes
Copy link
Contributor Author

coopernetes commented Sep 25, 2023

I'm a bit confused about how to test this, can you provide the steps I should run in the terminal?

Try using npm link - that will simulate installing the package and should also install the new executable.

@coopernetes

This comment was marked as outdated.

@coopernetes coopernetes force-pushed the feat/runnable branch 5 times, most recently from b2c0c01 to 06b4c1d Compare September 25, 2023 23:13
@coopernetes
Copy link
Contributor Author

Latest changes are available now @JamieSlome @maoo - please take a look and let me know if any other updates are needed.

README.md Outdated Show resolved Hide resolved
@maoo
Copy link
Member

maoo commented Sep 27, 2023

Latest changes are available now @JamieSlome @maoo - please take a look and let me know if any other updates are needed.

I've tested the README steps, including the --config flag, and everything works as expected, great stuff; I've suggested few minor changes on README.md

I'd suggest to tweak the https://github.com/finos/git-proxy#clone-the-repository section and change it to:

$ git clone https://localhost:8000/<YOUR_GITHUB_USRERNAME>/git-proxy.git

Alternatively, we could do git remote add proxy http://localhost:8000/finos/git-proxy.git, though the former approach would demonstrate that the pull operation works through the proxy.

@coopernetes @JamieSlome - WDYT?

README.md Outdated Show resolved Hide resolved
@coopernetes
Copy link
Contributor Author

@maoo gonna leave the documentation updates to #299. Would like this PR to remain focused on closing #183 with @JamieSlome updates to follow

@JamieSlome
Copy link
Member

@coopernetes - sounds good. I've taken the crux of all of your documentation improvements in the README and added to the documentation PR 👍

@coopernetes
Copy link
Contributor Author

Any additional changes to make before merging?

@maoo
Copy link
Member

maoo commented Oct 6, 2023

Any additional changes to make before merging?

LGTM! @JamieSlome - if you're ok with it, we may proceed with the merge?

@JamieSlome
Copy link
Member

LGTM! 🍰

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 12, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: coopernetes / name: Thomas Cooper (84207ae)

@netlify
Copy link

netlify bot commented Oct 12, 2023

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 84207ae
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/6528c5f0205b510008d60e5b
😎 Deploy Preview https://deploy-preview-287--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JamieSlome
Copy link
Member

@coopernetes - can we get your e-mail address CLA approved or address the commit directly?


❌ The email address for the commit (06b4c1d) is not linked to the GitHub account...

@coopernetes coopernetes force-pushed the feat/runnable branch 3 times, most recently from dca4fa4 to bd79fd0 Compare October 13, 2023 04:11
- add "bin" to package.json to support running git-proxy as an
  executable. Largely taken from # 184 and fixes # 183
- added --config and --validate command line flags via yargs. These will
  allow custom user configurations with a default behaviour of loading
  from the current working dir and/or default settings. --validate uses
  JSON Schema to validate the config.
- update docs to reflect new CLI args and additional details of running
  the app via npx
- load default settings from a module instead of explicit file path
- add test for default & user setting merging
- bump @finos/git-proxy to 1.1.0
- Remove old X.509 certificate and private key, which I assume was
  included previously to run git-proxy with TLS enabled via a demo. Can
  be re-added as needed but probably shouldn't be included in src (even
  if its for demo only).
@coopernetes
Copy link
Contributor Author

@JamieSlome corrected and no more conflicts from #299 - should be good at this point 👍

@JamieSlome JamieSlome merged commit c3c66b8 into finos:main Oct 13, 2023
9 checks passed
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.

Make installation and usage of the library easier
3 participants