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

feat: allow user to gets list of his tenants #353

Merged
merged 4 commits into from
Jan 23, 2024
Merged

feat: allow user to gets list of his tenants #353

merged 4 commits into from
Jan 23, 2024

Conversation

anatolychernov
Copy link
Contributor

PR by issue 328

@anatolychernov anatolychernov changed the title Allow user to gets list of his tenants feat : allow user to gets list of his tenants Dec 6, 2023
@anatolychernov anatolychernov changed the title feat : allow user to gets list of his tenants feat: allow user to gets list of his tenants Dec 6, 2023
@oliverbaehler oliverbaehler self-requested a review December 6, 2023 14:46
@oliverbaehler oliverbaehler added this to the v0.5.0 milestone Dec 6, 2023
@anatolychernov
Copy link
Contributor Author

Hello @oliverbaehler,

Could you clarify info about two failed checks:
Check Commit / commit_lint (pull_request) Failing after 11s
Go Lint / lint (pull_request) Failing after 1m

Should I fix something?

@anatolychernov
Copy link
Contributor Author

It seems I've fixed commit's checks

internal/modules/tenants/get.go Outdated Show resolved Hide resolved
internal/modules/tenants/list.go Outdated Show resolved Hide resolved
@anatolychernov
Copy link
Contributor Author

Hello,

Could you start other checks from workflow?

@prometherion
Copy link
Member

@anatolychernov golangci-lint is complaining since you're relying on the slices package which has been introduced with the v1.21.

We should first get the proxy update to this version of Go, then we can get this merged. Are you up to take care of this too?

@anatolychernov
Copy link
Contributor Author

@anatolychernov golangci-lint is complaining since you're relying on the slices package which has been introduced with the v1.21.

We should first get the proxy update to this version of Go, then we can get this merged. Are you up to take care of this too?

Hello,

I've updated Go version to 1.21 in go.mod

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

All the commits have the same subject, making hard to keep track of the changes.

Furthermore, Go 1.21 should be configured also on the CI actions, not only on the go.mod file.

e.g.: https://github.com/projectcapsule/capsule/blob/318d7d076a4a7640f6459ffc14d8e965380d344d/.github/workflows/e2e.yml#L46-L48

@anatolychernov
Copy link
Contributor Author

anatolychernov commented Dec 18, 2023

All the commits have the same subject, making hard to keep track of the changes.

Furthermore, Go 1.21 should be configured also on the CI actions, not only on the go.mod file.

e.g.: https://github.com/projectcapsule/capsule/blob/318d7d076a4a7640f6459ffc14d8e965380d344d/.github/workflows/e2e.yml#L46-L48

This link is from capsule project, not from capsule-proxy. But I've updated go version in Docker files.

@anatolychernov
Copy link
Contributor Author

@prometherion

If we don't have any more questions, can we proceed checks?

@prometherion
Copy link
Member

This link is from capsule project, not from capsule-proxy

The same Go runtime settings must be reported here:

jobs:
golangci:
name: lint
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Run golangci-lint
uses: golangci/golangci-lint-action@3a919529898de77ec3da873e3063ca4b10e7f5cc # v3.7.0
with:
version: v1.51.2
only-new-issues: false
args: --timeout 5m --config .golangci.yml

Otherwise, the golangci-lint won't be able to access the slices package from the standard library.

@anatolychernov
Copy link
Contributor Author

This link is from capsule project, not from capsule-proxy

The same Go runtime settings must be reported here:

jobs:
golangci:
name: lint
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Run golangci-lint
uses: golangci/golangci-lint-action@3a919529898de77ec3da873e3063ca4b10e7f5cc # v3.7.0
with:
version: v1.51.2
only-new-issues: false
args: --timeout 5m --config .golangci.yml

Otherwise, the golangci-lint won't be able to access the slices package from the standard library.

Hello @prometherion,

Do you mean something like this?

jobs:
  golangci:
    name: lint
    runs-on: ubuntu-20.04
    steps:
      - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
      - name: Install Go
        uses: actions/setup-go@v4
        with:
          go-version: '1.21'
          cache: false
      - name: Run golangci-lint
        uses: golangci/golangci-lint-action@3a919529898de77ec3da873e3063ca4b10e7f5cc # v3.7.0
        with:
          version: v1.51.2
          only-new-issues: false
          args: --timeout 5m --config .golangci.yml

@anatolychernov
Copy link
Contributor Author

@prometherion,

I've added "Install Go" step for lint.yaml

@anatolychernov
Copy link
Contributor Author

@prometherion, could you proceed checks to get 'Go lint' result?

@anatolychernov
Copy link
Contributor Author

anatolychernov commented Jan 4, 2024

Hello guys!
@prometherion,
I have rebased my changes to fresh main commits. Could you proceed other workflow checks?

@prometherion
Copy link
Member

@anatolychernov do you mind if you git push --force some fixes to get this merged?

@anatolychernov
Copy link
Contributor Author

@prometherion,
Do you mean that you want to combine these commits into one(squash) or I didn't understand you. Could you clarify it?

@prometherion
Copy link
Member

Maintainers are allowed to edit this pull request

I can push yo your branch to fix minor thingies, that would be much easier to get this merged, considering that everytime you have to ask for CI to get triggered since you're a first-time contributor 😅

@anatolychernov
Copy link
Contributor Author

Maintainers are allowed to edit this pull request

I can push yo your branch to fix minor thingies, that would be much easier to get this merged, considering that everytime you have to ask for CI to get triggered since you're a first-time contributor 😅

Yes, please do it. It will be great.

@anatolychernov
Copy link
Contributor Author

Maintainers are allowed to edit this pull request
I can push yo your branch to fix minor thingies, that would be much easier to get this merged, considering that everytime you have to ask for CI to get triggered since you're a first-time contributor 😅

Yes, please do it. It will be great.

@prometherion hello!
Do you have any updates about minor fixes?

@anatolychernov
Copy link
Contributor Author

I've signed off all my commits, but merging check is still blocked. Is it a glitch?

@prometherion
Copy link
Member

@anatolychernov I'm sorry for this tedious PR but I think we're there, eventually!

Since you're a first-time contributor, GI must approved by a project administrator, sorry for this.
Also, the latest golangci-lint seems having issues with the slices packages you used in your contribution: I changed it according to the current libraries we're using in capsule-proxy with the same outcome.
Furthermore, I spotted a bug in the Makefile which was still using the old golangci-lint binary.

Let's praise for the green CI 🙏🏻 and we can finally get this merged: thanks a lot for your patience and your efforts, I appreciate it!

prometherion and others added 2 commits January 23, 2024 18:42
Co-authored-by: Anatoly Chernov <[email protected]>
Signed-off-by: Dario Tranchitella <[email protected]>
Co-authored-by: Anatoly Chernov <[email protected]>
Signed-off-by: Dario Tranchitella <[email protected]>
prometherion and others added 2 commits January 23, 2024 18:42
Signed-off-by: Dario Tranchitella <[email protected]>
Co-authored-by: Anatoly Chernov <[email protected]>
Signed-off-by: Dario Tranchitella <[email protected]>
@prometherion prometherion merged commit 6ef70b3 into projectcapsule:main Jan 23, 2024
8 checks passed
@anatolychernov anatolychernov deleted the get_tenants branch January 23, 2024 19:41
@anatolychernov
Copy link
Contributor Author

@prometherion Thank you for help!

@oliverbaehler oliverbaehler modified the milestones: v0.5.0, 0.6.0 Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants