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

Add test fcr #48

Closed
wants to merge 21 commits into from
Closed

Add test fcr #48

wants to merge 21 commits into from

Conversation

tutkat
Copy link
Contributor

@tutkat tutkat commented Jul 26, 2024

For the PR were introduced changes:

  • changed api client (okhttp on rest-assured) and regenerated all classes
  • modified existing tests and adjusted for new client
  • added tests for Cloud Router endpoints
  • added test for Port endpoint
  • added flow to run tests only

@tutkat tutkat requested review from srushti-patl and thogarty July 26, 2024 17:07
@thogarty
Copy link
Contributor

@tutkat , please add descriptions of why you changed what you changed. The status update was confusing as well so it's important to add as much context as you can to whomever will be reviewing your code.

Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

It looks like this is a breaking change for existing users of the Java SDK. What will be the outcome to the user? Do they have to adjust their code based on this update?

We need to prevent breaking changes because our customers have to make many modifications as a result. We only make them if absolutely necessary and I still haven't seen enough evidence in your description to warrant the breaking change. What was the exact issue that okhttp-gson gave you that rest-assured is not?

.github/workflows/maven-publish.yml Outdated Show resolved Hide resolved
import io.swagger.v3.oas.models.PathItem;
import io.swagger.v3.oas.models.parameters.Parameter;
import io.swagger.v3.oas.models.parameters.Parameter.StyleEnum;
import io.swagger.v3.parser.OpenAPIV3Parser;
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a change in this template to modify the default timeouts returned by the API Client. You'll need to maintain that same default in the rest-assured API client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what "default timeouts" you mean?

v4/api/ConnectionsApiTest.java Outdated Show resolved Hide resolved
Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

Workflow needs to be modified based on comments.

.github/workflows/maven-publish.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

This is absolutely a breaking change requiring customer updates. Please provide ample evidence for this adjustment.

@tutkat
Copy link
Contributor Author

tutkat commented Aug 2, 2024

It looks like this is a breaking change for existing users of the Java SDK. What will be the outcome to the user? Do they have to adjust their code based on this update?

We need to prevent breaking changes because our customers have to make many modifications as a result. We only make them if absolutely necessary and I still haven't seen enough evidence in your description to warrant the breaking change. What was the exact issue that okhttp-gson gave you that rest-assured is not?

I don't know the reason but this client wasn't able to execute POST calls. I'm sure any client didn't do that because someone reports that doesn't work. I spent few days to try to configure it but unfortunately I was forced to move to another http client.

actually users after upgrade their usage need to upgrade imports and one method to perform call. actually example is put to readme

@ctreatma
Copy link
Contributor

ctreatma commented Aug 2, 2024

I don't know the reason but this client wasn't able to execute POST calls.

How does this issue manifest? Are the requests never sent at all? Are they sent in an invalid format and failing? Is the API responding with an HTTP error? Are all POST calls impacted or only for particular endpoints? The metal-java SDK also uses okhttp-gson, and although metal-java is now defunct it was definitely used successfully to send POST requests, which indicates it should be possible for fabric-java to send POST requests successfully without introducing a breaking change to the client.

@thogarty
Copy link
Contributor

@tutkat, please remove code generation from this PR. You need to figure out how to separate your test files from any changes you've made to the SDK during this time. There shouldn't be changes to 1,209 files just to add your tests.

@thogarty
Copy link
Contributor

@tutkat , this PR cannot be reviewed until it has been trimmed.

# Conflicts:
#	.github/workflows/auto-commit-client-updates.yml
#	.github/workflows/maven-publish.yml
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/JSON.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/api/MarketplaceSubscriptionsApi.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/api/PrecisionTimeApi.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/CloudRouter.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/CloudRouterPostRequest.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/Connection.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/ConnectionPostRequest.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/MarketplaceSubscription.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/PhysicalPort.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/PrecisionTimeServiceResponse.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/ServiceSearchResponse.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionAsset.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionAssetType.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionEntitlementResponse.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionResponse.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionRouterPackageType.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionStatus.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/SubscriptionTrial.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceFilter.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceFilters.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceOrFilter.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceSimpleExpression.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceSortBy.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceSortCriteria.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServiceSortDirection.java
#	equinix-openapi-fabric/src/main/java/com/equinix/openapi/fabric/v4/model/TimeServicesSearchRequest.java
@tutkat tutkat closed this Aug 13, 2024
@tutkat tutkat deleted the addTestFCR branch September 16, 2024 07:35
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