-
Notifications
You must be signed in to change notification settings - Fork 19
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
base: v1
Are you sure you want to change the base?
Conversation
joyent.com -> tritondatacenter.com
joyent.zone -> triton.zone, remove dead domains, update data center name to us-central-1
There was a problem hiding this 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 .
network/network_test.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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
`https?://(.*).api.samsungcloud.io`, | ||
`https?://(.*).api.tritoncompute.cloud`, | ||
`https?://(.*).api.mnx.io`, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
client/client_test.go
Outdated
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
compute/datacenters_test.go
Outdated
"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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No such thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
compute/images_test.go
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed.
Acceptance tests pass in
|
There was a problem hiding this 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.
network/network_test.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
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. |
One important thing to note is that merging this PR will make a new |
(Partially) fixes #202.
This PR is based on the
v1
branch. A separate PR will make the necessary changes for themaster
branch forv2
.Testing notes:
make test
passes (unit tests)make check
passes (formatting tests)make testacc
passes (acceptance / integration tests)