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

Allow passing Type and Uplink in registration requests #60

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

nkinkade
Copy link
Contributor

@nkinkade nkinkade commented Nov 27, 2024

Currently, both Type and Uplink in v2.Registration are statically set to "unknown". This change allows (requires) that valid-looking values for those fields be passed in registration commands.

We will need to be sure that RNP has updated their configurations before we release this, and also that our own test instances. Alternatively, I could update these changes to make these fields optional. I chose to make them required because it puts autojoined machines at parity with every other machine on the platform, where those fields are always present.


This change is Reviewable

Previously "Type" and "Uplink" in the v2.Registration struct were hardcoded as
"unknown". This change allows, and even requires, that the registration request
has nomimally valid values for those to fields.
@coveralls
Copy link
Collaborator

coveralls commented Nov 27, 2024

Pull Request Test Coverage Report for Build 12059126093

Details

  • 36 of 36 (100.0%) changed or added relevant lines in 2 files are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.8%) to 98.142%

Files with Coverage Reduction New Missed Lines %
handler/handler.go 10 97.8%
Totals Coverage Status
Change from base Build 12041337049: -0.8%
Covered Lines: 1215
Relevant Lines: 1238

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @nkinkade)


handler/handler.go line 187 at r1 (raw file):

	param.Type = req.URL.Query().Get("type")
	if !isValidType(param.Type) {
		fmt.Printf("bad machine type")

Prefer log if necessary. For log messages include more context; here for example the provided type value.


handler/handler.go line 189 at r1 (raw file):

		fmt.Printf("bad machine type")
		resp.Error = &v2.Error{
			Type:   "?machine-type=<machine-type>",

This error message references a parameter that is different from the one named in Get() above. What is the parameter name?

Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

PTAL?

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)


handler/handler.go line 187 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Prefer log if necessary. For log messages include more context; here for example the provided type value.

Sorry. This statement was actually just a debug line I added while updating the unit tests, and I never meant for it to make it through to the PR. It is removed now.


handler/handler.go line 189 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

This error message references a parameter that is different from the one named in Get() above. What is the parameter name?

Another good catch. I originally has this defined as Param.MachineType, since "type" is so generic. But after the fact I decided that if Type was good enough for v2.Registration, than it was good enough in the Param struct` too, and that having them equal made it easier to understand. I missed modifying this, but it is updated now.

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nkinkade nkinkade merged commit 1928e10 into main Nov 27, 2024
8 checks passed
@nkinkade nkinkade deleted the sandbox-kinkade branch November 27, 2024 22:52
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