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: non-interactive prebuild add/update #1217

Merged

Conversation

Abiji-2020
Copy link
Contributor

@Abiji-2020 Abiji-2020 commented Oct 8, 2024

Added the feature for the flags in prebuild add/update

Description

In the cobra command added the flags

Usage of update command after updating

daytona update <Project Config>  <PrebuildID>  --branch=main --retention=300 --commit-interval=60 --trigger-files="file1.go,file2.go" --run

Usage of the add command after updating

daytona prebuild add <Project Config>  --branch=main --retention=30 --commit-interval=60 --trigger-files="file1.go,file2.go" --run  
  • This change requires a documentation update
  • I have made corresponding changes to the documentation

Related Issue(s)

This PR fixes #985

Screenshots

If relevant, please add screenshots.

Notes

Please add any relevant notes if necessary.

@quest-bot loot #985

@Abiji-2020 Abiji-2020 requested a review from a team as a code owner October 8, 2024 16:28
Copy link

quest-bot bot commented Oct 8, 2024

Quest PR submitted! image Quest PR submitted!

@Abiji-2020, you are attempting to solve the issue and loot this Quest. Will you be successful?


Questions? Check out the docs.

@quest-bot quest-bot bot added the ⚔️ Quest Tracks quest-bot quests label Oct 8, 2024
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
…the project-config is required while running in non-interacive mode and for the update command the same goes along with the check using the interactive mode

Signed-off-by: Abinand P <[email protected]>
@Abiji-2020
Copy link
Contributor Author

@Tpuljak updated the code with the given requests and also modified for better working

@idagelic
Copy link
Member

@Abiji-2020 Two checks seem to be failing. To resolve them:

  1. Run golangci-lint run inside of the devcontainer to check for lint errors and try fixing those
  2. Run ./hack/generate-cli-docs.sh to generate the docs

@Abiji-2020 Abiji-2020 requested a review from a team as a code owner October 10, 2024 13:45
@Abiji-2020
Copy link
Contributor Author

@Abiji-2020 Two checks seem to be failing. To resolve them:

  1. Run golangci-lint run inside of the devcontainer to check for lint errors and try fixing those
  2. Run ./hack/generate-cli-docs.sh to generate the docs

@idagelic Updated the docs by running and resolved the issue caused in the linting

image

@Abiji-2020
Copy link
Contributor Author

@idagelic I am very sorry that after removing the debugging printing statement, I forgot to format the code. Which resulted in the failing of the tests

@Abiji-2020
Copy link
Contributor Author

@Tpuljak further reviews on the PR ?

@Tpuljak
Copy link
Member

Tpuljak commented Oct 11, 2024

@Abiji-2020 please do not mark conversations as resolved yourself. We will mark them as such after confirming they are resolved.

@Abiji-2020
Copy link
Contributor Author

@Abiji-2020 please do not mark conversations as resolved yourself. We will mark them as such after confirming they are resolved.

okay, I thought that after updating the necessary , then we need to resolve it. My apologies for that.

pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Show resolved Hide resolved
@Abiji-2020
Copy link
Contributor Author

add.and.update.commands.mp4

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.

image

Getting this when trying to update the prebuild using flags

pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
@Abiji-2020
Copy link
Contributor Author

image

Getting this when trying to update the prebuild using flags

sorry for that, I had created a new instance instead of passing the existing instance. Updated on the recent commit.

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.

Still having issues with updating. Do these steps work for you?

image

pkg/cmd/prebuild/add.go Outdated Show resolved Hide resolved
@Abiji-2020
Copy link
Contributor Author

Still having issues with updating. Do these steps work for you?

image

actually this is wanted to discuss that even at the stable current main this is same as it.

@idagelic
Copy link
Member

Still having issues with updating. Do these steps work for you?
image

actually this is wanted to discuss that even at the stable current main this is same as it.

I see. I'll take a look and push a fix to main first then.

@Abiji-2020
Copy link
Contributor Author

@idagelic

prebuild added

image

prebuild updated

image

on the original main branch of daytonaio. not this current branch

@idagelic
Copy link
Member

Fixed with #1244 - you can rebase with main and test once again to make sure everything works

@Abiji-2020
Copy link
Contributor Author

ok after testing it out, I will let you know

@Abiji-2020
Copy link
Contributor Author

@idagelic on checking the tests they are currently running as expected

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.

Looks like a format check is failing - you can fix it by running go fmt ./...

image

Updating using flags is not working as expected right now - I am unable to "patch" a single property - either commit-interval or trigger-files without it setting the other one to nil.

I also left a comment at the point in the code that you should check

pkg/cmd/prebuild/update.go Outdated Show resolved Hide resolved
…d server to filter the current config required when more then one branch have the same commit

Signed-off-by: Abinand P <[email protected]>
@Abiji-2020
Copy link
Contributor Author

@idagelic I also have modified in https://github.com/daytonaio/daytona/blob/main/pkg/server/projectconfig/prebuild.go#L29 to include the ID when checking for the existing id, as when we have multiple prebuild for a branch and a project only top most is retrieved and the exact id is not retrieved to update.

I made this change assuming that for a projectConfig and branch we can have multiple prebuilds.

@Abiji-2020
Copy link
Contributor Author

@idagelic I also have modified in https://github.com/daytonaio/daytona/blob/main/pkg/server/projectconfig/prebuild.go#L29 to include the ID when checking for the existing id, as when we have multiple prebuild for a branch and a project only top most is retrieved and the exact id is not retrieved to update.

I made this change assuming that for a projectConfig and branch we can have multiple prebuilds.

In this I guess I made a mistake that for a projectConfig and a same branch only one prebuild should be there, which is resolved now. Removed the conditon in the filter which was added by me in my previous commit

@Abiji-2020 Abiji-2020 force-pushed the non-interactive-flags-for-prebuild-add/update branch from 33b2eb6 to ca0170c Compare October 15, 2024 10:27
@Abiji-2020
Copy link
Contributor Author

I forgot to sign on while editing in the Github, so i rebased and signed it and force pushed

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 the contribution! Another reviewer will take a look before we complete the merge

@Tpuljak Tpuljak merged commit d62f68a into daytonaio:main Oct 15, 2024
12 checks passed
@Tpuljak Tpuljak changed the title feature: Added the support for the flags feat: non-interactive prebuild add/update Oct 15, 2024
Copy link

quest-bot bot commented Oct 15, 2024

🧚 @Abiji-2020 congratulations for completing Quest #985

💰 A reward of $50 has been credited to you.

To claim your $50 reward follow the instructions here.

Questions? Check out the docs.

@Abiji-2020
Copy link
Contributor Author

@Tpuljak @idagelic Thanks so much for guiding on this ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚔️ Quest Tracks quest-bot quests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-interactive prebuild adding and updating
3 participants