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.

Sign up for GitHub

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

Jump to bottom

add api call getSubnet #1633

Closed
wants to merge 4 commits into from
Closed

add api call getSubnet #1633

wants to merge 4 commits into from

Conversation

najeal
Copy link
Contributor

@najeal najeal commented Jun 19, 2023

Why this should be merged

It is related to issue #1255

How this works

It gets subnet tx from an ID.

How this was tested

Tested in service_test.go through mock.

@StephenButtolph
Copy link
Contributor

StephenButtolph commented Jun 19, 2023 •
edited
Loading

Thanks for opening this!

I'm wondering if it would be sufficient for the added API to only be GetSubnetTransformationTx, since GetTx could already be used to return the CreateSubnetTx.

Although maybe you were trying to avoid any pain around users trying to tell the difference between a permissioned subnet and a networking error.

@StephenButtolph StephenButtolph added enhancement New feature or request vm This involves virtual machines labels Jun 19, 2023
@StephenButtolph StephenButtolph linked an issue Jun 19, 2023 that may be closed by this pull request
Add an API call to check isElasticSubnet #1255
Closed
@StephenButtolph StephenButtolph changed the base branch from master to dev June 19, 2023 23:25
@najeal
Copy link
Contributor Author

najeal commented Jun 20, 2023

The idea was to have a generic call to get subnet then depending of the transaction being either CreateSubnetTx or TransformSubnetTx the user knows if the subnet is an elastic one or not.

Yes the user can already use GetTx() to get the CreateSubnetTx, but at this moment the user still needs to do an extra call to getSubnet(or GetSubnetTransformationTx) to know if it has been transformed.

What do you think @StephenButtolph It's up to you.

patrick-ogrady
patrick-ogrady reviewed Jun 21, 2023
View reviewed changes
vms/platformvm/service.go Outdated
errMissingPrivateKey = errors.New("argument 'privateKey' not given")
errStartAfterEndTime = errors.New("start time must be before end time")
errStartTimeInThePast = errors.New("start time in the past")
errUnexpectedTransactionType = fmt.Errorf("unexpected transaction type")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use errors.New here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ceyonur
ceyonur reviewed Jun 23, 2023
View reviewed changes
Copy link
Contributor

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

just some nits

vms/platformvm/service.go Outdated Show resolved Hide resolved
vms/platformvm/service.go Outdated Show resolved Hide resolved
vms/platformvm/service.go Outdated Show resolved Hide resolved
@najeal
Copy link
Contributor Author

najeal commented Aug 20, 2023

Any update?

@github-actions
Copy link

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

@najeal
Copy link
Contributor Author

najeal commented May 30, 2024

Closing this PR. #2704 is merged and resolves the issue

@najeal najeal closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrick-ogrady patrick-ogrady patrick-ogrady left review comments

@ceyonur ceyonur ceyonur left review comments

@abi87 abi87 Awaiting requested review from abi87

@danlaine danlaine Awaiting requested review from danlaine

@StephenButtolph StephenButtolph Awaiting requested review from StephenButtolph StephenButtolph is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request lifecycle/stale vm This involves virtual machines
Projects
Platform Engineering Group
Archived in project
Ski Patrol
Status: In Review 👀
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an API call to check isElasticSubnet
4 participants
@najeal @StephenButtolph @patrick-ogrady @ceyonur