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: multi-profile support for git providers(#777) #1032

Merged
merged 9 commits into from
Oct 9, 2024

Conversation

the-johnwick
Copy link
Contributor

@the-johnwick the-johnwick commented Aug 31, 2024

Support for Multiple Git Provider Tokens and Auto-Selection

Description

This update allows users to add multiple tokens for the same Git provider and specify which token to use with the --git-provider-config-id="..." flag or let the system auto-select the appropriate token.

Key Changes:

  1. Git Provider Config Struct:

    • Alias: Token alias for better identification.
    • ProviderId: Git provider name (e.g., GitHub, GitLab).
    • The Git provider ID is now a randomly generated UUID.
  2. Project and Project Config:

    • Added GitProviderConfigId field to associate a specific Git provider config with the project or project config.
  3. Token Selection Logic:

    • Fetch the repository context; if successful, it uses that token for the project. If an error occurs, it moves to the next token.

Breaking Change:

  • The Id of the Git provider config is now a randomly generated UUID.
  • ProviderId now represents the Git provider name instead of an identifier.

  • This change requires a documentation update
  • I have made corresponding changes to the documentation

This PR addresses issue #777
/claim #777

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

@the-johnwick the PR is ready for review but there's nothing in the description and one of the checks is failing.

@the-johnwick
Copy link
Contributor Author

the-johnwick commented Sep 2, 2024

@Tpuljak yes I already commented the problem #777 (comment)

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

That comment should go on the PR and not the issue.

Please include a description in the PR before requesting a review. The description should be filled out according to the template and be enough to describe all the changes made in the PR.

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

Did you run /hack/generate-cli-docs.sh?

@the-johnwick
Copy link
Contributor Author

@Tpuljak Sure I will add detailed description.

@the-johnwick
Copy link
Contributor Author

Did you run /hack/generate-cli-docs.sh?

Yes, twice

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

Screenshot 2024-09-02 at 10 29 55

It really does output a diff for me when running.

Are you maybe developing daytona inside of daytona?

@the-johnwick
Copy link
Contributor Author

Screenshot 2024-09-02 at 10 29 55

It really does output a diff for me when running.

Are you maybe developing daytona inside of daytona?

@Tpuljak No, I wasn't using Daytona and in last commit those two files were modified but still failling the checks

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

Screenshot 2024-09-02 at 10 29 55
It really does output a diff for me when running.
Are you maybe developing daytona inside of daytona?

@Tpuljak No, I wasn't using Daytona and in last commit those two files were modified but still failling the checks

Please rerun it and commit (without forcing) separately.

@the-johnwick
Copy link
Contributor Author

@Tpuljak ok I will try

@the-johnwick
Copy link
Contributor Author

the-johnwick commented Sep 2, 2024

@Tpuljak No differnce. it is still failing with the same error

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

@the-johnwick can you please record your screen while running

./hack/generate-cli-docs.sh
git status

@the-johnwick
Copy link
Contributor Author

./hack/generate-cli-docs.sh

sure i will record

@the-johnwick
Copy link
Contributor Author

the-johnwick commented Sep 2, 2024

@Tpuljak
Copy link
Member

Tpuljak commented Sep 2, 2024

@Tpuljak Please check https://github.com/user-attachments/assets/45a09f06-af6d-4353-9c77-d0975e55a2de

Thanks for the video. I noticed that you're running this on windows. That's the issue. Please run with GOOS=linux set. The issue is that we exclude some files from the windows build so they're not generated in the docs in your case.

@the-johnwick
Copy link
Contributor Author

@Tpuljak Please check https://github.com/user-attachments/assets/45a09f06-af6d-4353-9c77-d0975e55a2de

Thanks for the video. I noticed that you're running this on windows. That's the issue. Please run with GOOS=linux set. The issue is that we exclude some files from the windows build so they're not generated in the docs in your case.

I will try it again 🙂

@the-johnwick
Copy link
Contributor Author

@Tpuljak You check the PR now I have added the description and all checks are passing now.

@the-johnwick the-johnwick marked this pull request as draft September 3, 2024 15:19
@the-johnwick the-johnwick marked this pull request as ready for review September 3, 2024 15:20
@the-johnwick
Copy link
Contributor Author

@Tpuljak You can check the PR now I have resolved the merge conflicts and added the detailed description of the PR

Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Started with the review but I got a FATAL error when running dtn gp ls.

I had git providers set on the main branch and then switched to yours.

I assume that this branch is a breaking change due to changes in the GitProviderConfig struct.

Please add a breaking change description to the PR and BREAKING CHANGE: <description> above the signoff in the commit (you should squash the commits into one). The description in the PR should include steps on how to act after upgrading, while the description in the commit should only include what "broke".

docs/daytona_create.md Outdated Show resolved Hide resolved
@the-johnwick
Copy link
Contributor Author

@Tpuljak Thank you for the review. Could you please share a screenshot of the error so I can better understand what happened? The expected behavior is that it should work perfectly across all branches since the changes I made are branch-independent. Additionally, I believe we don't have separate Git configurations for different branches from the same repository.

@the-johnwick
Copy link
Contributor Author

the-johnwick commented Oct 2, 2024

Git provider

gp.mp4

Project Config Add

pc.mp4

Project creation

create.mp4

pkg/server/gitproviders/gitprovider.go Outdated Show resolved Hide resolved
@the-johnwick
Copy link
Contributor Author

the-johnwick commented Oct 7, 2024

@Tpuljak Can I Spilt this PR in to part since it is big PR and it implements more than one features and it would be easy for you to review as well.

@Tpuljak
Copy link
Member

Tpuljak commented Oct 7, 2024

@Tpuljak Can I Spilt this PR in to part since it is big PR and it implements more than one features and it would be easy for you to review as well.

There are more than 180 conversation items here and it's close to done. No need to split this into 2 PRs since it's only one feature that it implements.

pkg/server/workspaces/create.go Show resolved Hide resolved
pkg/server/gitproviders/gitprovider.go Outdated Show resolved Hide resolved
pkg/cmd/gitprovider/list.go Show resolved Hide resolved
BREAKING CHANGES: Remove All git provider config and readd it.

Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
@the-johnwick the-johnwick force-pushed the git-provider branch 2 times, most recently from 81c6bda to e0824f3 Compare October 9, 2024 09:19
Signed-off-by: John Wick <[email protected]>
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Nice work on this. Everything seems to be working as it should now.

Added 3 more small comments that should be addressed and then I can approve.

pkg/server/gitproviders/gitprovider.go Outdated Show resolved Hide resolved
pkg/server/projectconfig/dto/projectconfig.go Outdated Show resolved Hide resolved
pkg/views/gitprovider/select.go Outdated Show resolved Hide resolved
Signed-off-by: John Wick <[email protected]>
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

Thank you for your time! @the-johnwick

@idagelic idagelic merged commit ea5adb5 into daytonaio:main Oct 9, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants