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

refactor: removal of the manyminds/api2go, part 2 #2313

Merged
merged 2 commits into from
Feb 9, 2022
Merged

refactor: removal of the manyminds/api2go, part 2 #2313

merged 2 commits into from
Feb 9, 2022

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Dec 28, 2021 •
edited
Loading

What this PR does

It is a second PR related to the cleaning of api2go/jsonapi dependency. Once the client's usage is removed from the other projects, we can merge this.

Scope

The scope of this PR is to remove the api2go/jsonapi from the k6.

Related PRs

#2304

Closes: #911

@olegbespalov olegbespalov changed the title refactor: removal of the manyminds/api2go refactor: removal of the manyminds/api2go, part 2 Dec 28, 2021
@olegbespalov olegbespalov self-assigned this Dec 28, 2021
@olegbespalov olegbespalov mentioned this pull request Dec 28, 2021
Merged
@olegbespalov olegbespalov requested review from codebien and mstoykov and removed request for na-- December 28, 2021 15:25
codebien
codebien reviewed Dec 28, 2021
View reviewed changes
Copy link
Contributor

@codebien codebien left a comment •
edited
Loading

Choose a reason for hiding this comment

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

When do we expect to merge this? Will it be merged after the v0.36.0 release?

@olegbespalov
Copy link
Contributor Author

@codebien, to be honest, I don't have a sure answer, probably yes, since even the first PR in a row is under the changes.

But, why?

@codebien
Copy link
Contributor

We already added the deprecate warning in the first PR and this PR is removing it, so I didn't get why if we would merge it in the same release.

@olegbespalov
Copy link
Contributor Author

We can merge it in the same release, but only once we're sure that no other components are using the client.Call

@codebien
Copy link
Contributor

I don't see the value to merge the part 1 without this part. I think we should just have a unique PR and maintain it blocked until all the calls to the Call method haven't been removed. But let's wait for more opinions.

@olegbespalov
Copy link
Contributor Author

olegbespalov commented Dec 30, 2021 •
edited
Loading

I don't see the value to merge the part 1 without this part. I think we should just have a unique PR and maintain it blocked until all the calls to the Call method haven't been removed. But let's wait for more opinions.

The main thing why it splited into two parts is because it's a breaking change. It's usually a good practice to make such changes. Once the first part is merged, the other components have time to be migrated without a rush. Once the other components are fully migrated to the new interface, PR can be merged.

oleiade
oleiade previously approved these changes Jan 4, 2022
View reviewed changes
@na-- na-- added this to the v0.36.0 milestone Jan 5, 2022
mstoykov
mstoykov previously approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder if we should let there be a version with the deprecations and then removing it ... 🤔 I kind of doubt there are many users and I doubt they will even notice if it doesn't get removed so 🤷

mstoykov
mstoykov reviewed Jan 5, 2022
View reviewed changes
api/v1/group.go
}

// GetReferences returns the slice of jsonapi.References
// Deprecated: use instead g.Groups properties
Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to keep the deprecation for a version it seems to me like // Deprecated needs to be the last comment and separated with a newline from the rest in order for it to work ... but I might be wrong.

@olegbespalov olegbespalov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Jan 10, 2022
Base automatically changed from feat/911-remove-dependencies to master January 10, 2022 13:57
@olegbespalov olegbespalov dismissed stale reviews from mstoykov and oleiade via 24cb4f1 January 11, 2022 10:45
@olegbespalov olegbespalov modified the milestones: v0.36.0, v0.37.0 Jan 11, 2022
oleiade
oleiade previously approved these changes Jan 17, 2022
View reviewed changes
@codebien codebien mentioned this pull request Jan 17, 2022
Merged
codebien
codebien previously approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

mstoykov
mstoykov previously approved these changes Jan 28, 2022
View reviewed changes
oleiade
oleiade previously approved these changes Jan 31, 2022
View reviewed changes
@olegbespalov olegbespalov dismissed stale reviews from oleiade, mstoykov, and codebien via 42c1d84 January 31, 2022 10:48
oleiade
oleiade approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🍾

@codebien
Copy link
Contributor

codebien commented Feb 8, 2022

+2 −1,541

The best PRs 🎉

@olegbespalov olegbespalov merged commit 07ed4fe into master Feb 9, 2022
@olegbespalov olegbespalov deleted the feat/911-remove-dependencies-2 branch February 9, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codebien codebien codebien approved these changes

@oleiade oleiade oleiade approved these changes

@mstoykov mstoykov mstoykov approved these changes

Assignees

@olegbespalov olegbespalov

Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Milestone
v0.37.0
Development

Successfully merging this pull request may close these issues.

Remove dependencies for the API that are not necessary
5 participants
@olegbespalov @codebien @mstoykov @oleiade @na--