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: provider requirement handling #1300

Merged
merged 16 commits into from
Nov 22, 2024
Merged

feat: provider requirement handling #1300

merged 16 commits into from
Nov 22, 2024

Conversation

Philip-21
Copy link
Contributor

@Philip-21 Philip-21 commented Oct 28, 2024

Description

Requirement checks are added for providers before daytona starts, also a continuation of #1222

This PR addresses issue #673

Related Issue(s)

This PR addresses issue #673

BREAKING CHANGES

Before running the Server again, all providers will need to be removed by running the following command:

Linux:

rm -rf ~/.config/daytona/providers/*-provider/*-provider

Mac:

rm -rf ~/Library/Application\ Support/daytona/providers/*-provider/*-provider

Windows (PowerShell):

Remove-Item -Recurse -Force "C:\Users\<USERNAME>\AppData\Roaming\daytona\providers\*-provider\*-provider.exe"

After updating and starting the server. You can reinstall your providers with daytona provider install

@Philip-21 Philip-21 requested a review from a team as a code owner October 28, 2024 09:00
Signed-off-by: Philip-21 <[email protected]>
@Philip-21
Copy link
Contributor Author

Philip-21 commented Oct 28, 2024

@Tpuljak please have a look, i have been able to implement them in the rpc, and client setups, but i havent yet implemeted it in the main cli yet, i was having come nil pointer error when i tried testing it out locally , so i think its best the setups get merged then i can perform the calls to the checkrequirement() before daytona runs,

i have also pushed a pr in the docker provider

this is what i am referring to in the server.go , i will add this to handle the requirement checks

               infoColor := color.New(color.FgHiBlue).SprintFunc()
		warningColor := color.New(color.FgRed).SprintFunc()
		p := RequirementProvider{}

		requirements, err := p.Provider.CheckRequirements()
		if err != nil {
			return err
		}
		if requirements == nil {

		}

		for _, req := range *requirements {

			if req.Met {
				allRequirementsMet = true
				fmt.Printf("%s[0000]     Requirement met: %s\n", infoColor("INFO"), req.Reason)
			} else {
				allRequirementsMet = false
				fmt.Printf("%s[0000]  Requirement not met: %s\n", warningColor("WARNING"), req.Reason)
			}
			if !allRequirementsMet {
				return fmt.Errorf("    Daytona server startup aborted, one or more requirement not met")
			}
		}

Signed-off-by: Philip-21 <[email protected]>
@Tpuljak
Copy link
Member

Tpuljak commented Oct 28, 2024

@Philip-21 nice work. It seems to be going in the right direction.

this is what i am referring to in the server.go , i will add this to handle the requirement checks

This should also be added in this PR. We will release Daytona and the Providers on the same day so both should be in sync.

fmt.Printf("%s[0000]

Also, instead of this, use log.Info and log.Warn.

P.S. You will need to raise a PR in all our available providers for this to go through.

@Philip-21
Copy link
Contributor Author

@Philip-21 nice work. It seems to be going in the right direction.

Thank you

@Philip-21
Copy link
Contributor Author

P.S. You will need to raise a PR in all our available providers for this to go through.

Alright, i will add for the other providers to, i have to just look into them and study their sdk abit, since these other providers are cloud based .

@Philip-21
Copy link
Contributor Author

This should also be added in this PR. We will release Daytona and the Providers on the same day so both should be in sync.

fmt.Printf("%s[0000]

Also, instead of this, use log.Info and log.Warn.

Alright this is fine,

@Philip-21
Copy link
Contributor Author

Philip-21 commented Oct 28, 2024

so @Tpuljak it means we have to implement for all other providers, if not i can't perform the call to the checkrequirement() properly before daytona runs, now we have only the requiremnent for docker it wouldnt work, we have to implement for AWS, digital ocean and fly , because i tried it calling the checkrequirement() locally and i had nil pointer panic errors. It only worked when i hard-coded it in the first commits i pushed to the closed pr #1222 , and that doesn't follow our architectural procedure.

@Tpuljak
Copy link
Member

Tpuljak commented Oct 30, 2024

so @Tpuljak it means we have to implement for all other providers, if not i can't perform the call to the checkrequirement() properly before daytona runs, now we have only the requiremnent for docker it wouldnt work, we have to implement for AWS, digital ocean and fly , because i tried it calling the checkrequirement() locally and i had nil pointer panic errors. It only worked when i hard-coded it in the first commits i pushed to the closed pr #1222 , and that doesn't follow our architectural procedure.

Yes, you will have to implement the method in all other providers to satisfy the interface.

@Philip-21
Copy link
Contributor Author

so @Tpuljak it means we have to implement for all other providers, if not i can't perform the call to the checkrequirement() properly before daytona runs, now we have only the requiremnent for docker it wouldnt work, we have to implement for AWS, digital ocean and fly , because i tried it calling the checkrequirement() locally and i had nil pointer panic errors. It only worked when i hard-coded it in the first commits i pushed to the closed pr #1222 , and that doesn't follow our architectural procedure.

Yes, you will have to implement the method in all other providers to satisfy the interface.

They have all been implemented and referenced here

pkg/cmd/server/server.go Outdated Show resolved Hide resolved
@Philip-21
Copy link
Contributor Author

So it should be performed in the registerProviders() directly

@Tpuljak
Copy link
Member

Tpuljak commented Nov 4, 2024

So it should be performed in the registerProviders() directly

Yes

@Philip-21
Copy link
Contributor Author

@Tpuljak I have made changes

pkg/cmd/server/server.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/server/providers.go Outdated Show resolved Hide resolved
pkg/server/providers.go Outdated Show resolved Hide resolved
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
pkg/server/providers.go Outdated Show resolved Hide resolved
pkg/server/providers.go Outdated Show resolved Hide resolved
pkg/server/providers.go Outdated Show resolved Hide resolved
pkg/server/providers.go Outdated Show resolved Hide resolved
Signed-off-by: Philip-21 <[email protected]>
@Tpuljak
Copy link
Member

Tpuljak commented Nov 4, 2024

@Philip-21 code looks good. Nice work!

Since this requires all our providers to be updated as well, we won't merge this today. Instead, we can approve and merge after our release on Tuesday and queue this for Friday's release.

@Philip-21
Copy link
Contributor Author

Philip-21 commented Nov 4, 2024

Alright thanks alot @Tpuljak for putting me through.
I'm beginning to understand the architecture bit by bit

@Tpuljak
Copy link
Member

Tpuljak commented Nov 4, 2024

Alright thanks alot @Tpuljak for putting me through. I beginning to understand the architecture bit by bit

No problem. Glad to see that!

@Tpuljak
Copy link
Member

Tpuljak commented Nov 15, 2024

@Philip-21 can you please link all provider PRs in a single comment. Additionally, since we released the GCP and Hetzner providers in the meantime, they should also have a PR for this.

@Philip-21
Copy link
Contributor Author

@Philip-21 can you please link all provider PRs in a single comment. Additionally, since we released the GCP and Hetzner providers in the meantime, they should also have a PR for this.

Alright I am on it . I would implement the new providers and link everything here

@Philip-21
Copy link
Contributor Author

@Tpuljak these are the Pr's for handling requirement checks for all Daytona providers

Docker
GCP
AWS
Azure
Digital Ocean
FLY
Hetzner

@Tpuljak Tpuljak marked this pull request as ready for review November 22, 2024 08:28
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! Apologies for the long wait for approval. We'll get this merged and released today. Thanks!

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.

@Philip-21 please address these 2 comments before we give final approval.

pkg/provider/rpc_client.go Outdated Show resolved Hide resolved
pkg/provider/rpc_server.go Outdated Show resolved Hide resolved
Signed-off-by: Philip-21 <[email protected]>
@Philip-21
Copy link
Contributor Author

@Philip-21 please address these 2 comments before we give final approval.

Nice work! Apologies for the long wait for approval. We'll get this merged and released today. Thanks!

No problem, i have attended to the changes requested

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!

@Tpuljak Tpuljak changed the title Provider requirement handling feat: provider requirement handling Nov 22, 2024
@Tpuljak Tpuljak merged commit ea2db7f into daytonaio:main Nov 22, 2024
12 checks passed
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