obs: implement interfaces & CRUD operations for packages #255

nitishfy · 2023-10-07T16:59:43Z

Implement CRUD operations for OBS packages and Interfaces

k8s-ci-robot · 2023-10-07T16:59:45Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

xmudrii

Overall looks good, but some changes are required to get this merged

obs/project.go

xmudrii · 2023-10-16T12:09:59Z

obs/project.go

+
+type obsClient struct {
+	*http.Client
+	*Options


Options are not needed here

InvokeOBSEndpoint function has a receiver of type obsClient and we need to access the username and password to set the authentication. Hence this is the reason why Options have been included here.

IMO in that case the InvokeOBSEndpoint function should take username and password as parameters (cc @cpanato)

yeah, i think that should be a parameter as well

also, did you create a small testing cli code to use this and test it manually to check if you were able to do things?

obs/project.go

obs/packages.go

obs/project.go

obs/packages.go

xmudrii · 2023-10-17T08:19:42Z

obs/packages.go

+	pkg := &Package{}
+	if err := xml.NewDecoder(resp.Body).Decode(&pkg); err != nil {
+		return nil, fmt.Errorf("listing obs packages: decoding packages: %w", err)
+	}


This is a bit strange. This function is supposed to return a list of packages, but we're decoding the output into a single package. Can you please double check what is returned by the API?

How about now?

obs/packages.go

obs/project.go

obs/packages.go

obs/project.go

xmudrii · 2023-10-18T10:19:47Z

obs/packages.go

+}
+
+// ListPackages lists packages in a given OBS project
+func (o *OBS) ListPackages(ctx context.Context, projectName string) ([]string, error) {


This function is not going to work. We can discuss further about that, but I recommend removing it for now so that we can get this PR merged.

I think finding a way to test this function can help here. However, we can move this to next PR

xmudrii

Some final remarks, but it's important to resolve these remarks before merging the PR. Once resolved, we can merge the PR

obs/packages.go

xmudrii · 2023-10-19T20:52:08Z

obs/packages.go

+	Title       string   `json:"title,omitempty" xml:"title,omitempty"`
+	Description string   `json:"description,omitempty" xml:"description,omitempty"`
+	Devel       Devel    `json:"devel" xml:"devel"`
+	Entries     []Entry  `json:"entry" xml:"entry"`


I don't see this present in API endpoints for creating/updating and getting packages, I recommend removing it for now or eventually marking it as omitempty

It's part of the meta file for packages - https://api.opensuse.org/apidocs/#/Sources%20-%20Packages/get_source__project_name___package_name___meta

xmudrii

/lgtm
/approve

k8s-ci-robot · 2023-10-23T12:27:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nitishfy, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xmudrii]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2023

k8s-ci-robot requested review from cpanato and saschagrunert October 7, 2023 16:59

k8s-ci-robot added sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 7, 2023

nitishfy force-pushed the Nitish/obs-testing branch 2 times, most recently from a077847 to cf2a6e4 Compare October 12, 2023 18:14

nitishfy changed the title ~~obs: implement testing for the library functions~~ obs: implement interfaces for the library functions Oct 12, 2023

xmudrii suggested changes Oct 16, 2023

View reviewed changes

k8s-ci-robot assigned xmudrii Oct 16, 2023

xmudrii suggested changes Oct 16, 2023

View reviewed changes

obs/project.go Outdated Show resolved Hide resolved

xmudrii suggested changes Oct 16, 2023

View reviewed changes

obs/project.go Outdated Show resolved Hide resolved

obs/project.go Outdated Show resolved Hide resolved

obs/project.go Outdated Show resolved Hide resolved

obs/project.go Outdated Show resolved Hide resolved

xmudrii suggested changes Oct 16, 2023

View reviewed changes

nitishfy force-pushed the Nitish/obs-testing branch from 35001cc to 5d0f837 Compare October 16, 2023 19:28

nitishfy changed the title ~~obs: implement interfaces for the library functions~~ obs: implement interfaces & CRUD operations for packages Oct 16, 2023

nitishfy marked this pull request as ready for review October 16, 2023 19:29

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2023

k8s-ci-robot requested review from puerco and salaxander October 16, 2023 19:30

xmudrii suggested changes Oct 17, 2023

View reviewed changes

obs/project.go Outdated Show resolved Hide resolved

nitishfy force-pushed the Nitish/obs-testing branch from bc01d79 to f61305a Compare October 18, 2023 06:02

xmudrii suggested changes Oct 18, 2023

View reviewed changes

xmudrii suggested changes Oct 19, 2023

View reviewed changes

nitishfy force-pushed the Nitish/obs-testing branch from b5caa07 to 90cfb2a Compare October 20, 2023 14:41

nitishfy requested a review from xmudrii October 20, 2023 14:41

feat: implement interfaces & crud operations for packages

f54e086

nitishfy force-pushed the Nitish/obs-testing branch from efd2587 to f54e086 Compare October 23, 2023 11:57

xmudrii approved these changes Oct 23, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

k8s-ci-robot merged commit bc78723 into kubernetes-sigs:main Oct 23, 2023

nitishfy deleted the Nitish/obs-testing branch October 23, 2023 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

obs: implement interfaces & CRUD operations for packages #255

obs: implement interfaces & CRUD operations for packages #255

nitishfy commented Oct 7, 2023 •

edited

Loading

k8s-ci-robot commented Oct 7, 2023

xmudrii left a comment

xmudrii Oct 16, 2023

nitishfy Oct 16, 2023 •

edited

Loading

xmudrii Oct 16, 2023

cpanato Oct 17, 2023

xmudrii Oct 17, 2023

nitishfy Oct 17, 2023

xmudrii Oct 18, 2023

nitishfy Oct 19, 2023

xmudrii left a comment

xmudrii Oct 19, 2023

nitishfy Oct 20, 2023

xmudrii left a comment

k8s-ci-robot commented Oct 23, 2023

obs: implement interfaces & CRUD operations for packages #255

obs: implement interfaces & CRUD operations for packages #255

Conversation

nitishfy commented Oct 7, 2023 • edited Loading

k8s-ci-robot commented Oct 7, 2023

xmudrii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishfy Oct 16, 2023 • edited Loading

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

xmudrii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmudrii left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 23, 2023

nitishfy commented Oct 7, 2023 •

edited

Loading

nitishfy Oct 16, 2023 •

edited

Loading