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

Disable unnecessary linter rules #369

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Disable unnecessary linter rules #369

merged 1 commit into from
Mar 13, 2024

Conversation

MichaelSnowden
Copy link
Contributor

What changed?

  • I disabled several linter rules which weren't helping and had a lot of comments in our code.
  • I checked that every one of these is necessary (commented it out and re-ran the linter)
  • I added rationale for all the rules and a link to the docs for it.

Why?

Breaking changes
No.

@MichaelSnowden MichaelSnowden requested review from a team as code owners March 13, 2024 20:48
Comment on lines 89 to 103
// Optimistically create or update a Nexus service based on provided version.
// To update an existing service, get the current service record via the `GetNexusIncomingService` API, modify it
// and submit to this API.
// Set version to 0 to create a new service.
// Returns the updated service with the updated version, which can be used for subsequent updates.
rpc CreateOrUpdateNexusIncomingService(CreateOrUpdateNexusIncomingServiceRequest) returns (CreateOrUpdateNexusIncomingServiceResponse) {
}

// Delete an incoming Nexus service by name.
rpc DeleteNexusIncomingService(DeleteNexusIncomingServiceRequest) returns (DeleteNexusIncomingServiceResponse) {
}

// List all Nexus incoming services in the cluster. Use next_page_token in the response for pagination.
rpc ListNexusIncomingServices(ListNexusIncomingServicesRequest) returns (ListNexusIncomingServicesResponse) {
}
Copy link
Member

Choose a reason for hiding this comment

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

Why did these calls get deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how, but I added them back

buf.lock Outdated
@@ -5,9 +5,7 @@ deps:
owner: googleapis
repository: googleapis
commit: 28151c0d0a1641bf938a7672c500e01d
digest: shake256:49215edf8ef57f7863004539deff8834cfb2195113f0b890dd1f67815d9353e28e668019165b9d872395871eeafcbab3ccfdb2b5f11734d3cca95be9e8d139de
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the effect of this?

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 don't know. It got auto-removed somehow. I added it back 🤷‍♂️

Comment on lines 89 to 103
// Optimistically create or update a Nexus service based on provided version.
// To update an existing service, get the current service record via the `GetNexusIncomingService` API, modify it
// and submit to this API.
// Set version to 0 to create a new service.
// Returns the updated service with the updated version, which can be used for subsequent updates.
rpc CreateOrUpdateNexusIncomingService(CreateOrUpdateNexusIncomingServiceRequest) returns (CreateOrUpdateNexusIncomingServiceResponse) {
}

// Delete an incoming Nexus service by name.
rpc DeleteNexusIncomingService(DeleteNexusIncomingServiceRequest) returns (DeleteNexusIncomingServiceResponse) {
}

// List all Nexus incoming services in the cluster. Use next_page_token in the response for pagination.
rpc ListNexusIncomingServices(ListNexusIncomingServicesRequest) returns (ListNexusIncomingServicesResponse) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to do that in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed

Comment on lines 149 to 151
string namespace = 3;
// Task queue to route requests to.
string target_task_queue = 4;
string task_queue = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume for Nexus there's no problem with breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

I just changed this to include the target because some validation logic on the server is incompatible with this change.

We should put these in fields in Spec object the same way the Cloud API did it.

I'm assuming you didn't want to make this change here @MichaelSnowden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidentally left in yeah

@MichaelSnowden MichaelSnowden merged commit 64ce34a into master Mar 13, 2024
3 checks passed
@MichaelSnowden MichaelSnowden deleted the snowden/lint branch March 13, 2024 23:05
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.

4 participants