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

TRITON-2469 Update joyent links to TritonDataCenter #201

Open
wants to merge 18 commits into
base: v1
Choose a base branch
from
Open

Conversation

teutat3s
Copy link
Member

@teutat3s teutat3s commented Dec 5, 2024

(Partially) fixes #202.
This PR is based on the v1 branch. A separate PR will make the necessary changes for the master branch for v2.

Testing notes:

  • make test passes (unit tests)
  • make check passes (formatting tests)
  • make testacc passes (acceptance / integration tests)

@teutat3s teutat3s requested review from danmcd, bahamat and tsoome December 5, 2024 14:53
@teutat3s teutat3s changed the title Update joyent links to TritonDataCenter Update joyent links to TritonDataCenter #202 Dec 5, 2024
Copy link

@danmcd danmcd left a comment

Choose a reason for hiding this comment

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

Apart from anything I mention near changes...

  • Do you need to update CHANGELOG with the corrections, and any other versioning bump info? At worst it's a .z change (this would be 1.8.6). My go-fu is weak, so I'm not sure what the best answer here is.

  • Please file a jira ticket, or ask for me to do so. I want your testing notes gathered in there, even if they're already duplicated here or on Links should no longer point to github.com/joyent or joyent.com domain #202 .

@@ -29,7 +30,7 @@ var (
testNetworkID = ""
)

// Note that this is specific to Joyent Public Cloud and will not pass on
// Note that this is specific to Triton Public Cloud and will not pass on
Copy link

Choose a reason for hiding this comment

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

Don't you mean MNX.io? Do we still have these specifics in MNX.io?

Copy link
Member

Choose a reason for hiding this comment

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

This test hasn't been updated since exodus from Joyent. I'm not even sure it's been run since then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Jira ticket is TRITON-2469. I'll prepare a version bump here as well.

Copy link

Choose a reason for hiding this comment

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

I'd resolve, but would like @bahamat to make sure as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not even clear what this test is checking. If it passes in MNX then I'm ok with replacing Joyent Public Cloud with mnx.io's Triton Public Cloud. If not, then probably just revert it and leave it saying "this only passes on joyent", until we get this test reworked for...whatever.

An even better case would be to change the comment to say "This only works in Triton clouds that are configured with ..." and whatever that criteria is. Any Triton cloud could be configured to match what Joyent had (and MNX mostly does these days). So in general it's better to list the specific configuration being tested rather than trying to limit to a single unique cloud that no one else can replicate.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment is no longer true, in the test output I shared both TestAccNetworks_List and TestAccNetworks_Get pass. I deleted the comment.

Copy link
Member

@bahamat bahamat left a comment

Choose a reason for hiding this comment

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

Copyright needs to be MNX Cloud, Inc. in all cases. This isn't an exhaustive review.

Copy link
Member

@bahamat bahamat left a comment

Choose a reason for hiding this comment

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

In addition to other comments, all hard coded URLs should be eliminated in favor of user-supplied runtime variables.

client/client.go Outdated
Comment on lines 45 to 47
`https?://(.*).api.samsungcloud.io`,
`https?://(.*).api.tritoncompute.cloud`,
`https?://(.*).api.mnx.io`,
Copy link
Member

Choose a reason for hiding this comment

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

Remove samsung and tritoncompute.cloud. Honestly, the whole "short hand dc reference" should be eliminated entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been fixed. See comment below regarding removal of "short hand dc reference".

tsgEnv := "http://tsg.test.org"
jpcTritonURL := "https://us-east-1.api.joyent.com"
jpcTritonURL := "https://us-central-1.api.mnx.io"
spcTritonURL := "https://us-east-1.api.samsungcloud.io"
Copy link
Member

Choose a reason for hiding this comment

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

Remove all SPC references entirely.

This entire section should be reworked to not use any pre-defined URLs. It should only work with user supplied run-time defined account information.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are unit tests AFAICT. Would you like run-time defined account information for these?

"eu-ams-1": "https://eu-ams-1.api.joyentcloud.com",
"us-east-2": "https://us-east-2.api.joyentcloud.com",
"us-east-3": "https://us-east-3.api.joyentcloud.com"
"eu-ams-1": "https://eu-ams-1.api.tritoncompute.cloud",
Copy link
Member

Choose a reason for hiding this comment

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

No such thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

This test is actually pulling static images and comparing with known manifests. The changes in this file will break the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been fixed.

@teutat3s
Copy link
Member Author

teutat3s commented Dec 9, 2024

Acceptance tests pass in us-central-1, with these changes:

❯ make testacc
==> Running acceptance tests
TRITON_TEST=1 go test $(go list ./... |grep -Ev 'vendor|examples|testutils') -v -run TestAcc -timeout 60m
testing: warning: no tests to run
PASS
ok  	github.com/TritonDataCenter/triton-go	(cached) [no tests to run]
[...]
=== RUN   TestAccAccount_Get
--- PASS: TestAccAccount_Get (1.52s)
=== RUN   TestAccConfig_Get
--- PASS: TestAccConfig_Get (1.31s)
=== RUN   TestAccKey_Create
--- PASS: TestAccKey_Create (1.49s)
=== RUN   TestAccKey_Get
--- PASS: TestAccKey_Get (2.14s)
=== RUN   TestAccKey_Delete
--- PASS: TestAccKey_Delete (2.78s)
PASS
ok  	github.com/TritonDataCenter/triton-go/account	9.252s
testing: warning: no tests to run
PASS
ok  	github.com/TritonDataCenter/triton-go/client	(cached) [no tests to run]
=== RUN   TestAccDataCenters_List
--- PASS: TestAccDataCenters_List (1.33s)
=== RUN   TestAccDataCenters_Get
--- PASS: TestAccDataCenters_Get (0.82s)
=== RUN   TestAccImagesList
--- PASS: TestAccImagesList (1.32s)
=== RUN   TestAccImagesListInput
--- PASS: TestAccImagesListInput (1.14s)
=== RUN   TestAccImagesGet
--- PASS: TestAccImagesGet (1.10s)
=== RUN   TestAcc_Instance
=== RUN   TestAcc_Instance/InstanceGet
=== PAUSE TestAcc_Instance/InstanceGet
=== RUN   TestAcc_Instance/InstanceListTags
=== PAUSE TestAcc_Instance/InstanceListTags
=== RUN   TestAcc_Instance/InstanceListMetadata
=== PAUSE TestAcc_Instance/InstanceListMetadata
=== RUN   TestAcc_Instance/InstanceGetMetadata
=== PAUSE TestAcc_Instance/InstanceGetMetadata
=== RUN   TestAcc_Instance/InstanceUpdateMetadata
=== PAUSE TestAcc_Instance/InstanceUpdateMetadata
=== CONT  TestAcc_Instance/InstanceGet
=== CONT  TestAcc_Instance/InstanceListMetadata
=== CONT  TestAcc_Instance/InstanceListTags
=== CONT  TestAcc_Instance/InstanceGetMetadata
=== CONT  TestAcc_Instance/InstanceUpdateMetadata
--- PASS: TestAcc_Instance (28.32s)
    --- PASS: TestAcc_Instance/InstanceGet (1.80s)
    --- PASS: TestAcc_Instance/InstanceListMetadata (2.10s)
    --- PASS: TestAcc_Instance/InstanceListTags (2.32s)
    --- PASS: TestAcc_Instance/InstanceGetMetadata (2.74s)
    --- PASS: TestAcc_Instance/InstanceUpdateMetadata (2.77s)
=== RUN   TestAccPackagesList
--- PASS: TestAccPackagesList (0.86s)
=== RUN   TestAccPackagesListByName
--- PASS: TestAccPackagesListByName (0.88s)
=== RUN   TestAccPackagesGet
--- PASS: TestAccPackagesGet (0.87s)
=== RUN   TestAccServicesList
--- PASS: TestAccServicesList (0.80s)
PASS
ok  	github.com/TritonDataCenter/triton-go/compute	40.216s
testing: warning: no tests to run
PASS
ok  	github.com/TritonDataCenter/triton-go/errors	(cached) [no tests to run]
=== RUN   TestAccIdentity_SetRoleTags
--- PASS: TestAccIdentity_SetRoleTags (7.84s)
=== RUN   TestAccIdentity_GetRoleTags
--- PASS: TestAccIdentity_GetRoleTags (13.29s)
=== RUN   TestAccUser_ChangeUserPassword
--- PASS: TestAccUser_ChangeUserPassword (2.38s)
PASS
ok  	github.com/TritonDataCenter/triton-go/identity	23.513s
=== RUN   TestAccNetworks_List
--- PASS: TestAccNetworks_List (1.99s)
=== RUN   TestAccNetworks_Get
--- PASS: TestAccNetworks_Get (1.45s)
PASS
ok  	github.com/TritonDataCenter/triton-go/network	(cached)
testing: warning: no tests to run
PASS
ok  	github.com/TritonDataCenter/triton-go/services	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/TritonDataCenter/triton-go/storage	(cached) [no tests to run]

@teutat3s teutat3s changed the title Update joyent links to TritonDataCenter #202 TRITON-2469 Update joyent links to TritonDataCenter Dec 9, 2024
@teutat3s teutat3s requested review from bahamat and danmcd December 9, 2024 14:04
Copy link

@danmcd danmcd left a comment

Choose a reason for hiding this comment

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

Close to approving. I copied the test results into the ticket.

version.go Show resolved Hide resolved
@@ -29,7 +30,7 @@ var (
testNetworkID = ""
)

// Note that this is specific to Joyent Public Cloud and will not pass on
// Note that this is specific to Triton Public Cloud and will not pass on
Copy link

Choose a reason for hiding this comment

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

I'd resolve, but would like @bahamat to make sure as well.

@teutat3s
Copy link
Member Author

teutat3s commented Dec 9, 2024

In addition to other comments, all hard coded URLs should be eliminated in favor of user-supplied runtime variables.

This would be a change I would like to address in a separate PR, if that's okay? I would suggest bumping the version to 1.9.0 then.

@bahamat
Copy link
Member

bahamat commented Dec 14, 2024

In addition to other comments, all hard coded URLs should be eliminated in favor of user-supplied runtime variables.

This would be a change I would like to address in a separate PR, if that's okay? I would suggest bumping the version to 1.9.0 then.

Yeah, that would be fine, but I would like it to come very soon.

@teutat3s teutat3s requested a review from danmcd December 16, 2024 20:38
@teutat3s
Copy link
Member Author

One important thing to note is that merging this PR will make a new Go module / package appear at https://pkg.go.dev/github.com/TritonDataCenter/triton-go. Currently there's only https://pkg.go.dev/github.com/joyent/triton-go. Based on my research, because we do bump the triton-go version and thanks to GitHub redirecting all github.com/joyent links to github.com/TritonDataCenter, downstream projects using version 1.8.5 of this repo should continue to function after merging this.

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.

3 participants