confluence-go-api icon indicating copy to clipboard operation
confluence-go-api copied to clipboard

Add support for v2 API GetAttachmentById

Open coggsflod opened this issue 2 years ago • 6 comments

Checklist
  • [x] mage test passes
  • [x] unit tests are included and tested
  • [ ] documentation is added or changed (Please advise if documentation is preferred?)

coggsflod avatar Oct 26 '23 17:10 coggsflod

Hi team, I hope this will be a useful PR for folks. I haven't seen any dialogue with regard to v2 APIs on the project, so will await feedback on how to address the v1 / v2 transition. Thanks!

coggsflod avatar Oct 26 '23 17:10 coggsflod

hi @coggsflod, thank you for your pull request. Your PR is the first one for v2 api requests.

can you use v1 and v2 mixed together? Otherwise having a v2 version would make sense but this needs at least the commonly used functions to be implemented.

A good way would be to create a V2 branch and reimplement all existing endpoints to use v2, so we can merge it once the basic feature set is done and tag a v2 version.

This way we can prevent mixing up implementations and have a consistent transition to v2 api.

What do you think about this approach and would you like to invest a bit time into a v2 transition?

The current supported feature set is:

  • CRUD content
  • CRUD content templates
  • CRUD labels
  • Read comments, attachments, children of content objects, history, watchers and user information
  • Search via CQL

c-seeger avatar Nov 05 '23 09:11 c-seeger

Hi @c-seeger, you can absolutely mix v1 & v2 calls together as this is how I have implemented it in my project. I also believe that v2 isn't a complete replacement for v1 yet, so most folks requiring access to v2 would most likely require v1 also.

I wanted your feedback based on this info, before suggesting next steps?

Thanks!

coggsflod avatar Nov 08 '23 20:11 coggsflod

I have two questions:

Currently there are v1 attachment endpoints implemented e.g:

https://github.com/Virtomize/confluence-go-api/blob/5eab0c450474651e106f40da0af1e833deec9318/content.go#L248

as well as: https://github.com/Virtomize/confluence-go-api/blob/5eab0c450474651e106f40da0af1e833deec9318/content.go#L373 https://github.com/Virtomize/confluence-go-api/blob/5eab0c450474651e106f40da0af1e833deec9318/content.go#L382

GetAttachmentById replaces the first one, do you know if the other two can also be replaced by v2 api?

And the second one is about naming conventions:

Regarding the naming, we currently have the GetAttachments and GetAttachmentById which can be a bit confusing since its not clear that the first one relies on v1 API and second one on v2. I was thinking of replacing the GetAttachments function by your new implementation and move the old one to GetAttachmentsV1. But this would break the current API which would cause a major release change.

We could also add a version in between the calls:

e.g.

api, err := goconfluence.NewAPI("https://<your-domain>.atlassian.net/wiki/rest/api", "<username>", "<api-token>")
//... error handling

// instead of 
attachments, err := api.GetAttachments("<id>")

// v2 would be
attachment, err := api.v2.GetAttachments("<id>")

This would allow us to have full fallback compatibility and make it clear that the users uses v2 to GetAttachments. It also allows us to simply release a full v2 version once everything is implemented by just removing the v2 part to make it default.

I was thinking of sth. like

type API struct {
  endPoint        *url.URL
  Client          *http.Client
  username, token string
  v2 *v2
}

type v2 struct {
  // not sure if we need additional information here or leave it just empty
}

func (a *v2) GetAttachmentById(id string) (*Attachment, error) 

Whats your opinion on that. Does this make sense?

c-seeger avatar Nov 09 '23 07:11 c-seeger

I would also recommend to make a folder for v2 and put the implementation for v2 there

c-seeger avatar Nov 09 '23 07:11 c-seeger

also created a discussion for v2 Migration, always happy about a short review and some feedback.

c-seeger avatar Nov 09 '23 07:11 c-seeger