-
Notifications
You must be signed in to change notification settings - Fork 41
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 app-services-sdk-go in Core SDK #1664
base: main
Are you sure you want to change the base?
Updated to app-services-sdk-go in Core SDK #1664
Conversation
b5be255
to
e5e55ac
Compare
@@ -42,7 +42,7 @@ require ( | |||
github.com/prometheus/client_golang v1.14.0 | |||
github.com/prometheus/client_model v0.3.0 | |||
github.com/prometheus/common v0.42.0 | |||
github.com/redhat-developer/app-services-sdk-go/serviceaccounts v0.4.0 | |||
github.com/redhat-developer/app-services-sdk-core/app-services-sdk-go/serviceaccountmgmt v0.0.0-20230323122535-49460b57cc45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Quick question, there is no tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we are now publishing all packages in the SDK in one release the tag no longer points to the version (as that is the version of the parent project) but the commit/tag of the latest service account version.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest release is https://github.com/redhat-developer/app-services-sdk-core/releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to understand more. Does it mean that the tag 1.0.1 can't be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it is pointing to v1.0.1
just the tag is not it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was proposing is not to include the subpackage, but the whole package.
But I see you don't perform tagging at the module level. I wonder if what you are doing is in line with how Go module version management should be performed. The way it is being done users will never be able to use a specific module version via tag.
Maybe what you want are submodules, or not having tags per package (ocm sdk go doesn't do that for example)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they won't be able to set a specific tag, I am not sure what you mean by the rest of the comment.
What I was proposing is not to include the subpackage, but the whole package
I think this would be ideal but the rest of teh SDKs are structured like this and this is how the generator generates the code. There may be a way to change this but I will need to look into it. As for including only the top level module, I believe this still wouldn't fix the issue of not being able to specify the tag as the top level module is still a sub directory of the repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be ideal but the rest of teh SDKs are structured like this and this is how the generator generates the code.
What are the "rest of the SDKs"?
There may be a way to change this but I will need to look into it
👍
As for including only the top level module, I believe this still wouldn't fix the issue of not being able to specify the tag as the top level module is still a sub directory of the repo
Not having tags per "subdirectory" would be an approach. ocm sdk go doesn't do that for example, although it contains multiple subdirectories pertaining to different parts of the sdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the "rest of the SDKs"?
The TS, Java and Python SDKs
ocm sdk go doesn't do that for example
Yes looking at ocm-sdk-go
it just contains subdirectories for each service, this is different to use as they are different modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @jackdelahunt and @miguelsorianod to see if we reached a conclusion regarding the path forward for this PR.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1664 +/- ##
=======================================
Coverage 82.26% 82.26%
=======================================
Files 161 161
Lines 14989 14989
=======================================
Hits 12330 12330
Misses 2223 2223
Partials 436 436
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@jackdelahunt can you rebase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR, it needs a rebase @jackdelahunt
e5e55ac
to
1f18cca
Compare
Done 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @jackdelahunt for a rebase |
1f18cca
to
33bc316
Compare
@machi1990 done 👍 |
Description
This PR updates the location and version of the
app-services-sdk-go
used. The old package used here is now deprecated and is being replaced by the Core SDK.Verification Steps
Checklist (Definition of Done)