feat: expose the fleet manager version #1321

valeriiashapoval · 2022-09-20T17:05:02Z

Description

(https://issues.redhat.com/browse/MGDSTRM-9585)

Verification Steps

run make run from the root
run curl -H "Authorization: Bearer $(ocm token)" http://localhost:8000/api/kafkas_mgmt/v1 | jq .

Checklist (Definition of Done)

machi1990

thanks @valeriiashapoval for this.

I've added some suggestions. Let me know what you think?

can we also add an integration test?

pkg/api/metadata.go

machi1990 · 2022-09-21T06:41:11Z

pkg/api/metadata.go

+	if ok != nil {
+		version = ok.Error()
+	}


Maybe we could send the error instead as a 500 http code error?

Should we rename the ok return to err here? That's the default name for an error returned from a function. ok makes it sounds like it's a boolean.

@machi1990 what do you think about returning 500 from the endpoint if fetching the version failed? Returning the error reason in the response is a bit leaky and could lead to problems for whoever is parsing the response: they would have to know to be able to tell an error string from a commit hash string.

Thanks @pb82 @machi1990. Applied the changes!

what do you think about returning 500 from the endpoint if fetching the version failed?

The 500 response is returned as an error in json format so it can parsed.
Concretely, my suggestion is when an error occurs is to call
https://github.com/bf2fc6cc711aee1a0c2a/kas-fleet-manager/blob/ce9749f3a293242d72def0349a8e19d67fa65672/pkg/shared/handle_error.go#L11
and return

So the code here will look like

if err != nil { shared.HandleError(r,w,err) return }

@pb82 @valeriiashapoval

pkg/api/metadata.go

miguelsorianod · 2022-09-21T10:05:31Z

The changes seem to have an impact on the connectors code test. cc @valdar @dhirajsb.

This will also impact the connectors API.

machi1990

Please see #1321 (comment)

pkg/api/metadata.go

ziccardi · 2022-09-21T13:17:11Z

pkg/api/metadata.go

+		ID:            v.ID,
+		Kind:          "APIVersion",
+		HREF:          r.URL.Path,
+		ServerVersion: version,


According to the getCommitSha implementation, version here could be an empty string. Is that correct?

@machi1990 WDYT?

That's a good question.
It should never happen in our case ™️ but it is theoretically possible to not receive a commit sha: If the binary is built without the directory containing the git directory then the commit information would not be there. I don't know however in that case if ReadBuildInfo would return an error or not. I suspect it wouldn't return an error and simply the vcs.revision key would not exist or be empty.
I think it's a decision up to us. Should we always expect that vcs information be available/non-empty to be able start the server or we just set it as empty in that case ( which shouldn't happen in our use case).

It is acceptable that this is set to empty when we can't find the vcs info, this shouldn't happen for our case as @miguelsorianod pointed out since we copy the git directory in our binary building step in the dockerfile: https://github.com/bf2fc6cc711aee1a0c2a/kas-fleet-manager/blob/main/Dockerfile#L12.

We could of course verify this and or add an integration test for this.

ziccardi · 2022-09-21T13:19:19Z

pkg/api/metadata.go

 	"github.com/getsentry/sentry-go"
 	"github.com/golang/glog"
 )

+func getCommitSha() (string, error) {


Mind adding a comment explaining what this command does? (which commit sha it returns, when empty string is returned)

@valeriiashapoval can this method be documented as suggested in the previous comment?

pkg/api/metadata.go

codecov · 2022-09-27T13:30:16Z

Codecov Report

Merging #1321 (85b8803) into main (9595c5d) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 85b8803 differs from pull request most recent head 4fdc083. Consider uploading reports for the commit 4fdc083 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1321   +/-   ##
=======================================
  Coverage   83.65%   83.66%           
=======================================
  Files         139      139           
  Lines       12434    12436    +2     
=======================================
+ Hits        10402    10404    +2     
  Misses       1666     1666           
  Partials      366      366

Flag	Coverage Δ
unittests	`83.66% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ernal/workers/cluster_mgrs/dynamic_scale_up_mgr.go	`66.66% <0.00%> (+0.29%)`	⬆️

miguelsorianod · 2022-09-27T14:40:23Z

openapi/kas-fleet-manager.yaml

+        - id
+        - href
+      type: object
+      properties:


There is no need to duplicate the kind, id and href properties. The VersionMetadata object can be defined as a Composition of the ObjectReference object and a custom object inline that has the server_version and the collections attributes

Thanks @miguelsorianod ! The changes were implemented in the last commit.

miguelsorianod · 2022-09-27T14:43:37Z

internal/kafka/test/integration/metadata_test.go

+	TODO when the issue is resolved.
+	*/
+
+	g.Expect(version.ServerVersion).To(gomega.BeEmpty())


I think there is no value in testing that it is empty so I think we can omit this check and keep the comment

miguelsorianod · 2022-09-30T10:08:37Z

openapi/kas-fleet-manager.yaml

-                  - $ref: "#/components/schemas/ObjectReference"
+      - $ref: "#/components/schemas/ObjectReference"
+      - type: object
+        example:


The example should be updated to include server_version with an example value

Thanks a million @miguelsorianod ! Just committed the requested change.

miguelsorianod · 2022-09-30T12:26:18Z

pkg/api/metadata.go

 	"github.com/getsentry/sentry-go"
 	"github.com/golang/glog"
 )

+// getCommitSha() returns the current version (vcs.revision attribute of BuildInfo struct) using a built-in debug package and its ReadBuildInfo() function.
+// Upon error, an empty string is returned along with a GeneralError, as well as when vsc.revision is empty/unavailable.


// Upon error, an empty string is returned along with a GeneralError, as well as when vsc.revision is empty/unavailable.

This is not accurate.
When ReadBuildInfo returns successfully but vcs.revision is empty/unavailable then no error is returned

Thanks @miguelsorianod . Fixed now!

miguelsorianod

Looks good to me. Great work 👍

machi1990 · 2022-09-30T13:46:10Z

internal/kafka/test/integration/metadata_test.go

+	/*  Unable to test for a specific version as for now due to the debug package issue: https://github.com/golang/go/issues/33976,
+	TODO when the issue is resolved.
+	*/


Can we create a JIRA out of this?

Thanks @machi1990 ! Will create a Jira for it.

machi1990 · 2022-09-30T13:46:46Z

openapi/kas-fleet-manager.yaml

@@ -1966,4 +1969,4 @@ components:
          for cluster id: 1g5d88q0lrcdv4g7alb7slfgnj3dhbsj%!(EXTRA *errors.Error=identifier
          is '404', code is 'CLUSTERS-MGMT-404' and operation identifier is '1g5or50viu07oealuehrkc26dgftj1ac':
          Cluster '1g5d88q0lrcdv4g7alb7slfgnj3dhbsj' not found)"
-        operation_id: "1iYTsWry6nsqb2sNmFj5bXpD7Ca"
+        operation_id: "1iYTsWry6nsqb2sNmFj5bXpD7Ca"


Suggested change

operation_id: "1iYTsWry6nsqb2sNmFj5bXpD7Ca"

operation_id: "1iYTsWry6nsqb2sNmFj5bXpD7Ca"

nit adding newline at end of file

machi1990 · 2022-09-30T13:50:35Z

pkg/api/metadata.go

+	// Get the version:
+	version, err := getCommitSha()
+	if err != nil {
+		shared.HandleError(r, w, err.(*errors.ServiceError))


the err.(*errors.ServiceError) cast assumes all errors from getCommitSha will be service error. It is fine for now since only the GeneralError is returned. To be on the safe side I'd have used errors.ToServiceError(err) here.

machi1990

lgtm

left some minor comments for future considerations.
Let's also create a JIRA out of the TODO comment.
Thanks for the superb work @valeriiashapoval

valeriiashapoval · 2022-09-30T15:08:01Z

Thanks @miguelsorianod @machi1990 !
The JIRA for the TODO comment: https://issues.redhat.com/browse/MGDSTRM-9842

machi1990 · 2022-09-30T15:27:24Z

Thanks @valeriiashapoval and @miguelsorianod

When testing this change against a built KFM docker image, I see that the server_version is always empty.
However, the make run command works. I am current looking if it is just my setup or there is something missing on the current PR.

miguelsorianod · 2022-09-30T15:45:08Z

Thanks @valeriiashapoval and @miguelsorianod

When testing this change against a built KFM docker image, I see that the server_version is always empty. However, the make run command works. I am current looking if it is just my setup or there is something missing on the current PR.

As potentially useful information: build VCS information is only included if the .git directory is available when doing the build

…is read during build This follows up on bf2fc6cc711aee1a0c2a#1321, notably bf2fc6cc711aee1a0c2a#1321 (comment)

machi1990 · 2022-09-30T15:50:31Z

Thanks @valeriiashapoval and @miguelsorianod
When testing this change against a built KFM docker image, I see that the server_version is always empty. However, the make run command works. I am current looking if it is just my setup or there is something missing on the current PR.

As potentially useful information: build VCS information is only included if the .git directory is available when doing the build

Yes, this is the case already:

kas-fleet-manager/Dockerfile

Line 12 in 27db8c5

COPY . ./

I've found the missing piece, please have a look at #1333
@miguelsorianod @valeriiashapoval

valeriiashapoval requested review from a team, valdar and dhirajsb as code owners September 20, 2022 17:05

github-actions bot added the common label Sep 20, 2022

valeriiashapoval force-pushed the expose-the-kfm-version branch from c8f9438 to e0f2d86 Compare September 20, 2022 17:44

machi1990 reviewed Sep 21, 2022

View reviewed changes

ziccardi reviewed Sep 21, 2022

View reviewed changes

pkg/api/metadata.go Outdated Show resolved Hide resolved

ziccardi reviewed Sep 21, 2022

View reviewed changes

pkg/api/metadata.go Outdated Show resolved Hide resolved

valeriiashapoval force-pushed the expose-the-kfm-version branch from 11e945f to ce9749f Compare September 21, 2022 09:36

machi1990 requested changes Sep 21, 2022

View reviewed changes

valeriiashapoval force-pushed the expose-the-kfm-version branch 2 times, most recently from 7d14e4f to 922bfac Compare September 21, 2022 11:32

ziccardi reviewed Sep 21, 2022

View reviewed changes

pkg/api/metadata.go Show resolved Hide resolved

ziccardi reviewed Sep 21, 2022

View reviewed changes

pkg/api/metadata.go Outdated Show resolved Hide resolved

github-actions bot added api-change kafka labels Sep 26, 2022

valeriiashapoval force-pushed the expose-the-kfm-version branch 3 times, most recently from 5621144 to be120ce Compare September 27, 2022 13:09

valeriiashapoval requested a review from lburgazzoli as a code owner September 27, 2022 13:09

github-actions bot added the connector label Sep 27, 2022

miguelsorianod reviewed Sep 27, 2022

View reviewed changes

valeriiashapoval force-pushed the expose-the-kfm-version branch from 7b21397 to 04d940d Compare September 28, 2022 14:51

miguelsorianod reviewed Sep 30, 2022

View reviewed changes

valeriiashapoval force-pushed the expose-the-kfm-version branch from 37d96ea to 9923b3c Compare September 30, 2022 10:19

valeriiashapoval force-pushed the expose-the-kfm-version branch 2 times, most recently from bfc1ade to e113a57 Compare September 30, 2022 11:50

miguelsorianod reviewed Sep 30, 2022

View reviewed changes

feat: expose the fleet manager version

4fdc083

valeriiashapoval force-pushed the expose-the-kfm-version branch from 85b8803 to 4fdc083 Compare September 30, 2022 12:44

miguelsorianod approved these changes Sep 30, 2022

View reviewed changes

miguelsorianod requested a review from machi1990 September 30, 2022 13:29

machi1990 reviewed Sep 30, 2022

View reviewed changes

machi1990 approved these changes Sep 30, 2022

View reviewed changes

miguelsorianod merged commit 27db8c5 into bf2fc6cc711aee1a0c2a:main Sep 30, 2022

valeriiashapoval deleted the expose-the-kfm-version branch September 30, 2022 15:42

machi1990 mentioned this pull request Sep 30, 2022

build(dockerfile): ensure that docker in installed so that commitsha is read during build #1333

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose the fleet manager version #1321

feat: expose the fleet manager version #1321

valeriiashapoval commented Sep 20, 2022

machi1990 left a comment

machi1990 Sep 21, 2022

pb82 Sep 21, 2022

pb82 Sep 21, 2022

valeriiashapoval Sep 21, 2022

machi1990 Sep 21, 2022 •

edited

miguelsorianod commented Sep 21, 2022 •

edited

machi1990 left a comment

ziccardi Sep 21, 2022

ziccardi Sep 21, 2022

miguelsorianod Sep 21, 2022 •

edited

machi1990 Sep 21, 2022

miguelsorianod Sep 21, 2022

ziccardi Sep 21, 2022

miguelsorianod Sep 27, 2022

codecov bot commented Sep 27, 2022 •

edited

miguelsorianod Sep 27, 2022

valeriiashapoval Sep 30, 2022

miguelsorianod Sep 27, 2022

miguelsorianod Sep 30, 2022

valeriiashapoval Sep 30, 2022

miguelsorianod Sep 30, 2022

valeriiashapoval Sep 30, 2022

miguelsorianod left a comment

machi1990 Sep 30, 2022

valeriiashapoval Sep 30, 2022

machi1990 Sep 30, 2022

machi1990 Sep 30, 2022

machi1990 left a comment

valeriiashapoval commented Sep 30, 2022

machi1990 commented Sep 30, 2022

miguelsorianod commented Sep 30, 2022 •

edited

machi1990 commented Sep 30, 2022 •

edited

	operation_id: "1iYTsWry6nsqb2sNmFj5bXpD7Ca"
	operation_id: "1iYTsWry6nsqb2sNmFj5bXpD7Ca"

feat: expose the fleet manager version #1321

feat: expose the fleet manager version #1321

Conversation

valeriiashapoval commented Sep 20, 2022

Description

Verification Steps

Checklist (Definition of Done)

machi1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machi1990 Sep 21, 2022 • edited

Choose a reason for hiding this comment

miguelsorianod commented Sep 21, 2022 • edited

machi1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelsorianod Sep 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelsorianod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machi1990 left a comment

Choose a reason for hiding this comment

valeriiashapoval commented Sep 30, 2022

machi1990 commented Sep 30, 2022

miguelsorianod commented Sep 30, 2022 • edited

machi1990 commented Sep 30, 2022 • edited

machi1990 Sep 21, 2022 •

edited

miguelsorianod commented Sep 21, 2022 •

edited

miguelsorianod Sep 21, 2022 •

edited

codecov bot commented Sep 27, 2022 •

edited

miguelsorianod commented Sep 30, 2022 •

edited

machi1990 commented Sep 30, 2022 •

edited