Skip to content
This repository was archived by the owner on May 15, 2025. It is now read-only.

feat(jfrog): support multiple repositories #289

Merged
merged 11 commits into from
Sep 27, 2024
Merged

feat(jfrog): support multiple repositories #289

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
31c29fa
Fix check for shell completion
brent-at-aam Sep 12, 2024
646adbc
Support for multiple repos
brent-at-aam Sep 15, 2024
0361847
Fix function syntax
BrentSouza Sep 16, 2024
b6c0d58
Update readme and port to jfrog-oauth
brent-at-aam Sep 20, 2024
fd5ba0f
Port tests to oauth module and fix errors
brent-at-aam Sep 20, 2024
67c1d63
Fix formatting
brent-at-aam Sep 20, 2024
4b0ac2a
`bun fmt`
matifali Sep 20, 2024
d06cd4a
Address review items
brent-at-aam Sep 20, 2024
149cb2f
Remove accidental import
brent-at-aam Sep 20, 2024
70ec87f
Merge branch 'coder:main' into jfrog-enhancements
BrentSouza Sep 20, 2024
fb409cf
Merge branch 'main' into jfrog-enhancements
matifali Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions jfrog-oauth/.npmrc.tftpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
email=${ARTIFACTORY_EMAIL}
%{ for REPO in REPOS ~}
${REPO.SCOPE}registry=${JFROG_URL}/artifactory/api/npm/${REPO.NAME}
//${JFROG_HOST}/artifactory/api/npm/${REPO.NAME}/:_authToken=${ARTIFACTORY_ACCESS_TOKEN}
%{ endfor ~}
21 changes: 11 additions & 10 deletions jfrog-oauth/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

agent_id = coder_agent.example.id
jfrog_url = "https://example.jfrog.io"
username_field = "username" # If you are using GitHub to login to both Coder and Artifactory, use username_field = "username"

package_managers = {
"npm" : "npm",
"go" : "go",
"pypi" : "pypi"
npm = ["npm", "@scoped:npm-scoped"]
go = ["go", "another-go-repo"]
pypi = ["pypi", "extra-index-pypi"]
docker = ["example-docker-staging.jfrog.io", "example-docker-production.jfrog.io"]
}
}
```
Expand All @@ -44,13 +45,13 @@ Configure the Python pip package manager to fetch packages from Artifactory whil
```tf
module "jfrog" {
source = "registry.coder.com/modules/jfrog-oauth/coder"
version = "1.0.15"
version = "2.0.0"
agent_id = coder_agent.example.id
jfrog_url = "https://example.jfrog.io"
username_field = "email"

package_managers = {
"pypi" : "pypi"
pypi = ["pypi"]
}
}
```
Expand All @@ -72,15 +73,15 @@ The [JFrog extension](https://open-vsx.org/extension/JFrog/jfrog-vscode-extensio
```tf
module "jfrog" {
source = "registry.coder.com/modules/jfrog-oauth/coder"
version = "1.0.15"
version = "2.0.0"
agent_id = coder_agent.example.id
jfrog_url = "https://example.jfrog.io"
username_field = "username" # If you are using GitHub to login to both Coder and Artifactory, use username_field = "username"
configure_code_server = true # Add JFrog extension configuration for code-server
package_managers = {
"npm" : "npm",
"go" : "go",
"pypi" : "pypi"
npm = ["npm"]
go = ["go"]
pypi = ["pypi"]
}
}
```
Expand Down
114 changes: 105 additions & 9 deletions jfrog-oauth/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,115 @@
import { serve } from "bun";
import { describe } from "bun:test";
import { describe, expect, it } from "bun:test";
import {
createJSONResponse,
findResourceInstance,
runTerraformInit,
runTerraformApply,
testRequiredVariables,
} from "../test";

describe("jfrog-oauth", async () => {
await runTerraformInit(import.meta.dir);

testRequiredVariables(import.meta.dir, {
agent_id: "some-agent-id",
jfrog_url: "http://localhost:8081",
package_managers: "{}",
const fakeFrogHostAndPort = "localhost:8081";
const fakeFrogUrl = `http://${fakeFrogHostAndPort}`;

it("can run apply with required variables", async () => {
testRequiredVariables(import.meta.dir, {
Copy link
Member

Choose a reason for hiding this comment

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

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 */)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. This is not at all in the runtime path, we're just potentially saving a developer testing time
  2. Typos will bubble up as a result of running the mock terraform apply and the tests will fail
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

agent_id: "some-agent-id",
jfrog_url: fakeFrogUrl,
package_managers: "{}",
});
});
});

//TODO add more tests
it("generates an npmrc with scoped repos", async () => {
const state = await runTerraformApply(import.meta.dir, {
agent_id: "some-agent-id",
jfrog_url: fakeFrogUrl,
package_managers: JSON.stringify({
npm: ["global", "@foo:foo", "@bar:bar"],
}),
});
const coderScript = findResourceInstance(state, "coder_script");
const npmrcStanza = `cat << EOF > ~/.npmrc
[email protected]
registry=${fakeFrogUrl}/artifactory/api/npm/global
//${fakeFrogHostAndPort}/artifactory/api/npm/global/:_authToken=
@foo:registry=${fakeFrogUrl}/artifactory/api/npm/foo
//${fakeFrogHostAndPort}/artifactory/api/npm/foo/:_authToken=
@bar:registry=${fakeFrogUrl}/artifactory/api/npm/bar
//${fakeFrogHostAndPort}/artifactory/api/npm/bar/:_authToken=
EOF`;
expect(coderScript.script).toContain(npmrcStanza);
expect(coderScript.script).toContain(
'jf npmc --global --repo-resolve "global"',
);
expect(coderScript.script).toContain(
'if [ -z "YES" ]; then\n not_configured npm',
);
});

it("generates a pip config with extra-indexes", async () => {
const state = await runTerraformApply(import.meta.dir, {
agent_id: "some-agent-id",
jfrog_url: fakeFrogUrl,
package_managers: JSON.stringify({
pypi: ["global", "foo", "bar"],
}),
});
const coderScript = findResourceInstance(state, "coder_script");
const pipStanza = `cat << EOF > ~/.pip/pip.conf
[global]
index-url = https://default:@${fakeFrogHostAndPort}/artifactory/api/pypi/global/simple
extra-index-url =
https://default:@${fakeFrogHostAndPort}/artifactory/api/pypi/foo/simple
https://default:@${fakeFrogHostAndPort}/artifactory/api/pypi/bar/simple
EOF`;
expect(coderScript.script).toContain(pipStanza);
expect(coderScript.script).toContain(
'jf pipc --global --repo-resolve "global"',
);
expect(coderScript.script).toContain(
'if [ -z "YES" ]; then\n not_configured pypi',
);
});

it("registers multiple docker repos", async () => {
const state = await runTerraformApply(import.meta.dir, {
agent_id: "some-agent-id",
jfrog_url: fakeFrogUrl,
package_managers: JSON.stringify({
docker: ["foo.jfrog.io", "bar.jfrog.io", "baz.jfrog.io"],
}),
Comment on lines +93 to +95
Copy link
Member

Choose a reason for hiding this comment

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

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

});
const coderScript = findResourceInstance(state, "coder_script");
const dockerStanza = `register_docker "foo.jfrog.io"
register_docker "bar.jfrog.io"
register_docker "baz.jfrog.io"`;
expect(coderScript.script).toContain(dockerStanza);
expect(coderScript.script).toContain(
'if [ -z "YES" ]; then\n not_configured docker',
);
});

it("sets goproxy with multiple repos", async () => {
const state = await runTerraformApply(import.meta.dir, {
agent_id: "some-agent-id",
jfrog_url: fakeFrogUrl,
package_managers: JSON.stringify({
go: ["foo", "bar", "baz"],
}),
});
const proxyEnv = findResourceInstance(state, "coder_env", "goproxy");
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);
Copy link
Member

@Parkreiner Parkreiner Sep 20, 2024 •
edited by matifali
Loading

Choose a reason for hiding this comment

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

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(","));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this:

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that works, too


const coderScript = findResourceInstance(state, "coder_script");
expect(coderScript.script).toContain(
'jf goc --global --repo-resolve "foo"',
);
expect(coderScript.script).toContain(
'if [ -z "YES" ]; then\n not_configured go',
);
});
});
88 changes: 61 additions & 27 deletions jfrog-oauth/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,23 +53,51 @@ variable "configure_code_server" {
}

variable "package_managers" {
type = map(string)
description = <<EOF
A map of package manager names to their respective artifactory repositories.
For example:
{
"npm": "YOUR_NPM_REPO_KEY",
"go": "YOUR_GO_REPO_KEY",
"pypi": "YOUR_PYPI_REPO_KEY",
"docker": "YOUR_DOCKER_REPO_KEY"
}
EOF
type = object({
npm = optional(list(string), [])
go = optional(list(string), [])
pypi = optional(list(string), [])
docker = optional(list(string), [])
})
description = <<-EOF
A map of package manager names to their respective artifactory repositories. Unused package managers can be omitted.
For example:
{
npm = ["GLOBAL_NPM_REPO_KEY", "@SCOPED:NPM_REPO_KEY"]
go = ["YOUR_GO_REPO_KEY", "ANOTHER_GO_REPO_KEY"]
pypi = ["YOUR_PYPI_REPO_KEY", "ANOTHER_PYPI_REPO_KEY"]
docker = ["YOUR_DOCKER_REPO_KEY", "ANOTHER_DOCKER_REPO_KEY"]
}
EOF
}

locals {
# The username field to use for artifactory
username = var.username_field == "email" ? data.coder_workspace_owner.me.email : data.coder_workspace_owner.me.name
jfrog_host = replace(var.jfrog_url, "https://", "")
jfrog_host = split("://", var.jfrog_url)[1]
common_values = {
JFROG_URL = var.jfrog_url
JFROG_HOST = local.jfrog_host
JFROG_SERVER_ID = var.jfrog_server_id
ARTIFACTORY_USERNAME = local.username
ARTIFACTORY_EMAIL = data.coder_workspace_owner.me.email
ARTIFACTORY_ACCESS_TOKEN = data.coder_external_auth.jfrog.access_token
}
npmrc = templatefile(
"${path.module}/.npmrc.tftpl",
merge(
local.common_values,
{
REPOS = [
for r in var.package_managers.npm :
strcontains(r, ":") ? zipmap(["SCOPE", "NAME"], ["${split(":", r)[0]}:", split(":", r)[1]]) : { SCOPE = "", NAME = r }
]
}
)
)
pip_conf = templatefile(
"${path.module}/pip.conf.tftpl", merge(local.common_values, { REPOS = var.package_managers.pypi })
)
}

data "coder_workspace" "me" {}
Expand All @@ -83,19 +111,22 @@ resource "coder_script" "jfrog" {
agent_id = var.agent_id
display_name = "jfrog"
icon = "/icon/jfrog.svg"
script = templatefile("${path.module}/run.sh", {
JFROG_URL : var.jfrog_url,
JFROG_HOST : local.jfrog_host,
JFROG_SERVER_ID : var.jfrog_server_id,
ARTIFACTORY_USERNAME : local.username,
ARTIFACTORY_EMAIL : data.coder_workspace_owner.me.email,
ARTIFACTORY_ACCESS_TOKEN : data.coder_external_auth.jfrog.access_token,
CONFIGURE_CODE_SERVER : var.configure_code_server,
REPOSITORY_NPM : lookup(var.package_managers, "npm", ""),
REPOSITORY_GO : lookup(var.package_managers, "go", ""),
REPOSITORY_PYPI : lookup(var.package_managers, "pypi", ""),
REPOSITORY_DOCKER : lookup(var.package_managers, "docker", ""),
})
script = templatefile("${path.module}/run.sh", merge(
local.common_values,
{
CONFIGURE_CODE_SERVER = var.configure_code_server
HAS_NPM = length(var.package_managers.npm) == 0 ? "" : "YES"
NPMRC = local.npmrc
REPOSITORY_NPM = try(element(var.package_managers.npm, 0), "")
HAS_GO = length(var.package_managers.go) == 0 ? "" : "YES"
REPOSITORY_GO = try(element(var.package_managers.go, 0), "")
HAS_PYPI = length(var.package_managers.pypi) == 0 ? "" : "YES"
PIP_CONF = local.pip_conf
REPOSITORY_PYPI = try(element(var.package_managers.pypi, 0), "")
HAS_DOCKER = length(var.package_managers.docker) == 0 ? "" : "YES"
REGISTER_DOCKER = join("\n", formatlist("register_docker \"%s\"", var.package_managers.docker))
}
))
run_on_start = true
}

Expand All @@ -121,10 +152,13 @@ resource "coder_env" "jfrog_ide_store_connection" {
}

resource "coder_env" "goproxy" {
count = lookup(var.package_managers, "go", "") == "" ? 0 : 1
count = length(var.package_managers.go) == 0 ? 0 : 1
agent_id = var.agent_id
name = "GOPROXY"
value = "https://${local.username}:${data.coder_external_auth.jfrog.access_token}@${local.jfrog_host}/artifactory/api/go/${lookup(var.package_managers, "go", "")}"
value = join(",", [
for repo in var.package_managers.go :
"https://${local.username}:${data.coder_external_auth.jfrog.access_token}@${local.jfrog_host}/artifactory/api/go/${repo}"
])
}

output "access_token" {
Expand Down
6 changes: 6 additions & 0 deletions jfrog-oauth/pip.conf.tftpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[global]
index-url = https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_ACCESS_TOKEN}@${JFROG_HOST}/artifactory/api/pypi/${try(element(REPOS, 0), "")}/simple
extra-index-url =
%{ for REPO in try(slice(REPOS, 1, length(REPOS)), []) ~}
https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_ACCESS_TOKEN}@${JFROG_HOST}/artifactory/api/pypi/${REPO}/simple
%{ endfor ~}
Loading