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

Updated to be wharf-api v5.0.0 compatible #29

Merged
merged 41 commits into from
Jan 24, 2022

Conversation

Alexamakans
Copy link
Member

@Alexamakans Alexamakans commented Dec 9, 2021

Summary

  • Updated Go version to Go 1.16.
  • Changed functions to match wharf-api v5.0.0 handler names, without the -handler suffix.
  • Did some clean up, mainly through helper functions for making requests.
  • Now using structs from wharf-api request/response packages instead of defining them here.

Motivation

Supporting wharf-api v5.0.0 faster and more smoothly when it gets released.
Keep up-to-date.

Not implemented methods

Methods listed in #31 are not implemented here, as it is out of scope for this PR.


Closes #24, closes #25

@Alexamakans Alexamakans added the enhancement New feature or request label Dec 9, 2021
@Alexamakans Alexamakans self-assigned this Dec 9, 2021
@Alexamakans Alexamakans added this to In progress in Backlog via automation Dec 9, 2021
Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

Looks great! I'm having second-thoughts about adding the dependency on wharf-api, but I don't see it being resolve in any other way without adding uncessesary complexity with yet another repo or whatnot.

The Go modules compatibility stuff is something we have to look closer into. Maybe a short meeting to resolve it?

I added a bunch of comments regarding the new Client.Get, Client.GetDecoded, etc. methods. I find them in a limbo between adding simplified abstractions and still being tedious to work with. I'll have to take a second look later for more comments, but here a meeting could be benefitial as well

CHANGELOG.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/wharfapi/artifact.go Outdated Show resolved Hide resolved
pkg/wharfapi/requestHandler.go Outdated Show resolved Hide resolved
pkg/wharfapi/token.go Outdated Show resolved Hide resolved
pkg/wharfapi/client.go Outdated Show resolved Hide resolved
pkg/wharfapi/build.go Outdated Show resolved Hide resolved
pkg/wharfapi/client.go Outdated Show resolved Hide resolved
Backlog automation moved this from In progress to Review in progress Dec 10, 2021
pkg/wharfapi/requestHandler.go Outdated Show resolved Hide resolved
pkg/wharfapi/client.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/wharfapi/branch.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fredx30 fredx30 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Is the prefer var syntax in the linting rules?

A sugestion - mixing refactor commits and change/feature commits in one PR makes code generally more difficult to review as they are hard to tell apart and require a different logic set to review. The sugestion here for future times would be split them up- ex. make one PR that tackles refactors, one that tackles removal of unused code, and one that tackles implementation of new features.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@applejag
Copy link
Contributor

applejag commented Jan 18, 2022

Regarding this:

Functions not implemented yet, mostly because I don't think we need them, but also because I'm unsure on how to.

  • CreateBuildArtifact (Sending file(s) through multipart form data)
  • CreateBuildTestResult (Sending file(s) through multipart form data)
  • StreamBuildLog (Receiving a stream)

I'm OK with still leaving them out for this PR, as they were not implemented before either. Would be nice to have them implemented for v2, but I do not see that as a requirement and they could easily be postponed to v2.1.

Suggest creating an issue on it and then just referring to that in this PR, stating that it's out of scope for this PR

Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

The Client.get, .getUnmarshal, etc series of methods are very practical. Each endpoint method still looks a little bit boilerplated but it's soooo much better than before.

Great work!

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/model/response/response.go Outdated Show resolved Hide resolved
pkg/model/request/request.go Outdated Show resolved Hide resolved
pkg/wharfapi/requestHandler.go Outdated Show resolved Hide resolved
pkg/wharfapi/token.go Show resolved Hide resolved
Backlog automation moved this from Review in progress to Reviewer approved Jan 24, 2022
@Alexamakans Alexamakans merged commit 6701727 into master Jan 24, 2022
Backlog automation moved this from Reviewer approved to Done Jan 24, 2022
@Alexamakans Alexamakans deleted the feature/make-wharf-api-v5.0.0-compatible branch January 24, 2022 07:53
@Alexamakans Alexamakans mentioned this pull request Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Map projects by ID: Add RemoteProjectID Make wharf-api v5.0 compatible
3 participants