feat(jfrog): support multiple repositories #289

BrentSouza · 2024-09-16T16:18:46Z

This MR addresses two items:

There is a bug in run.sh that keeps adding autocompletion to either .bashrc or .zshrc
Allow users to specify multiple repositories for a given artifact type

Autocompletion

The run.sh script is looking for the wrong string to check if autocompletion has already been applied. The changes here correct that so autocompletion stanzas are only added once to the relevant shell startup script.

Multiple Repos

With Artifactory, you can have multiple repositories of a given type. For example, at my company, we promote docker images across environment specific repositories to keep our production image repository clean. It would be nice to setup auth for each of these instead of picking just one. The changes here allow for a list of repos to be defined. The first will function as a global repository, with varying behavior for the others based on the type.

pypi

The first repo is added to pip.conf as index-url, while any additional repos are added to extra-index-url

npm

The first repo is added as a default registry. Subsequent repos should be scoped, e.g. @foo:npm-repo-name and they will be added as such to .npmrc

docker

Attempts to authenticate to all defined repositories

go

Adds all repos to GOPROXY

I've added some tests to check the various output in the generated script, but I still need to validate that the module works as intended in our coder deployment. I wanted to open this pull request now to get any potential feedback on the changes. If this is ok, I'll also extend the changes to the jfrog-oauth module.

Signed-off-by: bsouza <[email protected]>

BrentSouza · 2024-09-17T11:15:17Z

Tested the module from my branch in our deployment and it's working at least how I intended! Please let me know if there are any changes you would like made.

matifali · 2024-09-17T13:15:45Z

Thank you, @BrentSouza, for the contribution. We have two modules for JFrog; one uses OAuth, and the other uses an admin token to create user-scoped access tokens. Would it be possible for you to port all these changes to the OAuth module, too?
Again this is a great addition.

BrentSouza · 2024-09-17T20:51:11Z

Thanks @matifali! Yes I will happily make the changes to jfrog-oauth. Do you want a separate PR for that one?

matifali · 2024-09-18T06:26:59Z

Let's keep them in a single PR.

matifali · 2024-09-19T15:17:10Z

Once you are done with the changes. I can test both of them and merge the PR.
Again thanks a lot for the contribution.

BrentSouza · 2024-09-20T13:30:15Z

@matifali at your convenience, please take a look at the port to jfrog oauth 😄. I can't test this one locally as we are on coder community edition and already have external auth configured with our GitLab instance.

BrentSouza · 2024-09-20T13:33:43Z

Actually I found a bug already 😱 Patch incoming

BrentSouza · 2024-09-20T13:51:29Z

Ok @matifali I think we should have successful test and pretty runs now

matifali · 2024-09-20T13:55:05Z

@BrentSouza Thank you for the contribution. This is a huge enhancement.

Need to test and re-review

Parkreiner

Checked all the test files, and while I had some feedback, they all look good

@matifali I'm going to defer to you on the Terraform files. I think they look good, but you definitely know more than I do. As long as you think they look good, I think this is pretty safe to approve

Parkreiner · 2024-09-20T18:44:18Z

jfrog-oauth/README.md

@@ -17,15 +17,16 @@ Install the JF CLI and authenticate package managers with Artifactory using OAut
 ```tf
 module "jfrog" {
  source         = "registry.coder.com/modules/jfrog-oauth/coder"
-  version        = "1.0.15"
+  version        = "2.0.0"


@matifali I'm still not fully clear on how we're handling versioning, but we would want this to be version 1.0.19?

yes, we will go with 1.0.19 for now until we resolve #157.

Ok. I was bumping the major version since the input variable format for the repos is a breaking change, but if there's an outside constraint, I'll update the readme to reflect the target version of 1.0.19

It is indeed a breaking change but we do not want to bump all future modules to 2.0.0 before we do #157.

Parkreiner · 2024-09-20T19:07:44Z

jfrog-oauth/main.test.ts

+  const fakeFrogUrl = `http://${fakeFrogHostAndPort}`;
+
+  it("can run apply with required variables", async () => {
+    testRequiredVariables(import.meta.dir, {


Just to provide a little more type-safety, could you define a custom type for the variables, and pass it along as a type parameter?

type TestVariables = Readonly<{ agent_id: string; jfrog_url: string; package_managers: string; username_field?: string; jfrog_server_id?: string; external_auth_id: string; configure_code_server?: boolean; }>;

You'd then be able to pass this type to both runTerraformApply and testRequiredVariables to catch any accidental typos:

runTerraformApply<TestVariables>(/* Same arguments as before */)

I appreciate the concept here but please allow me to push back slightly on this suggestion. I fear it might be redundant to define the variables in the test with minimal value because:

This is not at all in the runtime path, we're just potentially saving a developer testing time

Typos will bubble up as a result of running the mock terraform apply and the tests will fail

Since there is no definitive binding between the variables as written in tf to the variables in this type def, there's nothing to prevent someone from typing the variable name incorrectly in the typescript type def.

In summary, I think you are only validating the test code is type safe and not really validating the logic of the module.

I don't see this pattern in many, if any, of the other tests for modules but if you are adamant that this is the correct thing for your code, I will implement it.

I put this in place in the interest of getting these changes merged. I do think it feels contrived since you have to define the type and then make sure you specify the type as a type param for the various function calls to get the benefit of enforcing the type check.

It might be a tighter workflow if you had a generic Testing class instead of the free functions in test.ts. So you would do something like:

describe("some-module", async () => { type SomeVariables = { ... }; const test = new Testing<SomeVariables>(); const state = await test.runTerraformApply(...); ... });

Obviously a change outside the scope of this PR, but does that make sense to you?

Sorry, was a little swamped these past few days, and haven't had a chance to respond earlier

On one hand, I guess I view the current setup as not that different from other JavaScript values like Maps or Sets. If you have something like this:

const userMap = Map<string, User>();

I don't think that's too different from one of the testing functions. The type restrictions don't exist at runtime, so non-user values can still accidentally be placed in the Map.

But I think you're right, and I think the current type behavior is a bit of a bandaid. These are just test files, so them breaking on typos isn't as huge of a deal, because you get instant feedback before they bubble up to an end-user

But going to your idea about the generic testing class, that does seem like a really good idea. The reason why having Map<string, User> is valuable is because the type restriction trickles down to each individual method. You don't have to keep repeating the type info on every single call

I think I'll actually make a separate PR once this one is done to leverage classes more

Parkreiner · 2024-09-20T19:15:51Z

jfrog-oauth/main.test.ts

+    const proxies = `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/foo,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/bar,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/baz`;
+    expect(proxyEnv["value"]).toEqual(proxies);


I feel like this might be a little more readable:

Suggested change

const proxies = `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/foo,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/bar,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/baz`;

expect(proxyEnv["value"]).toEqual(proxies);

const proxies = [

`https://default:@${fakeFrogHostAndPort}/artifactory/api/go/foo`,

`https://default:@${fakeFrogHostAndPort}/artifactory/api/go/bar`,

`https://default:@${fakeFrogHostAndPort}/artifactory/api/go/baz`

];

expect (proxyEnv.value).toBe(proxies.join(","));

How about this:

["foo","bar","baz"].map(r => `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/${r}`).join(",");

Yeah, that works, too

Parkreiner · 2024-09-20T19:21:02Z

jfrog-oauth/main.test.ts

+      package_managers: JSON.stringify({
+        docker: ["foo.jfrog.io", "bar.jfrog.io", "baz.jfrog.io"],
+      }),


This definitely isn't your fault, but I'm making a note so I don't forget: we should update our helper functions so that they support Terraform objects directly

Parkreiner · 2024-09-20T19:36:15Z

jfrog-token/main.test.ts

+  const fakeFrogUrl = `http://${fakeFrogHostAndPort}`;
+
+  it("can run apply with required variables", async () => {
+    testRequiredVariables(import.meta.dir, {


Same comment as the auth test file. Looks like the type can be set up like this?

type TestVariables = Readonly<{ agent_id: string; jfrog_url: string; artifactory_access_token: string; jfrog_server_id?: string; token_description?: string; check_license?: boolean; refreshable?: boolean; expires_in?: number; configure_code_server?: boolean; package_managers?: string; }>;

Parkreiner · 2024-09-20T19:37:32Z

jfrog-token/main.test.ts

+    const proxies = `https://default:xxx@${fakeFrogHostAndPort}/artifactory/api/go/foo,https://default:xxx@${fakeFrogHostAndPort}/artifactory/api/go/bar,https://default:xxx@${fakeFrogHostAndPort}/artifactory/api/go/baz`;
+    expect(proxyEnv["value"]).toEqual(proxies);


Also feel like this could be split up for readability

- Fix versions in readme - Make expected test values readable - Support typed vars for testing

BrentSouza · 2024-09-20T21:24:47Z

I've addressed all concerns in the latest commit. I had to make some changes to test.ts to support typed vars since the method signature for testRequiredVariables was Record<string, string>

Parkreiner

Thank for being so patient with me. The tests look good, and also, I think your suggestion for how to update the general test approach makes a lot more sense than what we have right now

BrentSouza · 2024-09-26T13:02:45Z

No thanks necessary. Thanks for the taking the time to give valuable feedback!

brent-at-aam added 2 commits September 12, 2024 16:51

Fix check for shell completion

31c29fa

Signed-off-by: bsouza <[email protected]>

Support for multiple repos

646adbc

matifali self-requested a review September 16, 2024 18:06

Fix function syntax

0361847

matifali changed the title ~~feat(modules): JFrog Module Enhancements~~ feat(jfrog-token): JFrog Toke Module Enhancements Sep 17, 2024

BrentSouza changed the title ~~feat(jfrog-token): JFrog Toke Module Enhancements~~ feat(jfrog-token): JFrog Token Module Enhancements Sep 17, 2024

BrentSouza marked this pull request as ready for review September 17, 2024 11:14

matifali previously approved these changes Sep 17, 2024

View reviewed changes

matifali changed the title ~~feat(jfrog-token): JFrog Token Module Enhancements~~ feat(jfrog): support multiple repositories Sep 18, 2024

Update readme and port to jfrog-oauth

b6c0d58

brent-at-aam added 2 commits September 20, 2024 13:43

Port tests to oauth module and fix errors

fd5ba0f

Fix formatting

67c1d63

bun fmt

4b0ac2a

matifali mentioned this pull request Sep 20, 2024

chore: cleanup all test files #293

Merged

Parkreiner reviewed Sep 20, 2024

View reviewed changes

Address review items

d06cd4a

- Fix versions in readme - Make expected test values readable - Support typed vars for testing

brent-at-aam and others added 2 commits September 20, 2024 21:34

Remove accidental import

149cb2f

Merge branch 'coder:main' into jfrog-enhancements

70ec87f

matifali requested a review from Parkreiner September 23, 2024 08:36

Parkreiner approved these changes Sep 24, 2024

View reviewed changes

Merge branch 'main' into jfrog-enhancements

fb409cf

matifali merged commit ad1189a into coder:main Sep 27, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jfrog): support multiple repositories #289

feat(jfrog): support multiple repositories #289

BrentSouza commented Sep 16, 2024

BrentSouza commented Sep 17, 2024

matifali commented Sep 17, 2024

BrentSouza commented Sep 17, 2024

matifali commented Sep 18, 2024

matifali commented Sep 19, 2024 •

edited

Loading

BrentSouza commented Sep 20, 2024 •

edited

Loading

BrentSouza commented Sep 20, 2024

BrentSouza commented Sep 20, 2024

matifali commented Sep 20, 2024 •

edited

Loading

Parkreiner left a comment

Parkreiner Sep 20, 2024

matifali Sep 20, 2024

BrentSouza Sep 20, 2024

matifali Sep 20, 2024

Parkreiner Sep 20, 2024

BrentSouza Sep 20, 2024

BrentSouza Sep 20, 2024

Parkreiner Sep 24, 2024

Parkreiner Sep 24, 2024

Parkreiner Sep 20, 2024 •

edited by matifali

Loading

BrentSouza Sep 20, 2024

Parkreiner Sep 20, 2024

Parkreiner Sep 20, 2024

Parkreiner Sep 20, 2024

Parkreiner Sep 20, 2024

BrentSouza commented Sep 20, 2024

Parkreiner left a comment

BrentSouza commented Sep 26, 2024

		const proxies = `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/foo,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/bar,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/baz`;
		expect(proxyEnv["value"]).toEqual(proxies);

-    const proxies = `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/foo,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/bar,https://default:@${fakeFrogHostAndPort}/artifactory/api/go/baz`;
-    expect(proxyEnv["value"]).toEqual(proxies);
+const proxies = [
+  `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/foo`,
+  `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/bar`,
+  `https://default:@${fakeFrogHostAndPort}/artifactory/api/go/baz`
+];
+expect (proxyEnv.value).toBe(proxies.join(","));

		const proxies = `https://default:xxx@${fakeFrogHostAndPort}/artifactory/api/go/foo,https://default:xxx@${fakeFrogHostAndPort}/artifactory/api/go/bar,https://default:xxx@${fakeFrogHostAndPort}/artifactory/api/go/baz`;
		expect(proxyEnv["value"]).toEqual(proxies);

feat(jfrog): support multiple repositories #289

feat(jfrog): support multiple repositories #289

Conversation

BrentSouza commented Sep 16, 2024

Autocompletion

Multiple Repos

pypi

npm

docker

go

BrentSouza commented Sep 17, 2024

matifali commented Sep 17, 2024

BrentSouza commented Sep 17, 2024

matifali commented Sep 18, 2024

matifali commented Sep 19, 2024 • edited Loading

BrentSouza commented Sep 20, 2024 • edited Loading

BrentSouza commented Sep 20, 2024

BrentSouza commented Sep 20, 2024

matifali commented Sep 20, 2024 • edited Loading

Parkreiner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Sep 20, 2024 • edited by matifali Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrentSouza commented Sep 20, 2024

Parkreiner left a comment

Choose a reason for hiding this comment

BrentSouza commented Sep 26, 2024

matifali commented Sep 19, 2024 •

edited

Loading

BrentSouza commented Sep 20, 2024 •

edited

Loading

matifali commented Sep 20, 2024 •

edited

Loading

Parkreiner Sep 20, 2024 •

edited by matifali

Loading