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.

Sign up for GitHub

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

Jump to bottom

Display a warning that "odo dev" on Podman needs to be restarted if the Devfile is changed #6477

Merged
merged 6 commits into from
Jan 10, 2023
Merged

Display a warning that "odo dev" on Podman needs to be restarted if the Devfile is changed #6477

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffe78b6
Display a warning that "odo dev" on Podman might need to be restarted…
rm3l Jan 9, 2023
d45cfbd
Add integration test case
rm3l Jan 9, 2023
7b71a78
fixup! Add integration test case
rm3l Jan 9, 2023
6f51b75
amend! Display a warning that "odo dev" on Podman might need to be re…
rm3l Jan 9, 2023
8058649
fixup! Add integration test case
rm3l Jan 10, 2023
82ace2f
Assert that no warning is displayed if the Devfile has not changed
rm3l Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/dev/podmandev/podmandev.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/redhat-developer/odo/pkg/dev/common"
"github.com/redhat-developer/odo/pkg/devfile/adapters"
"github.com/redhat-developer/odo/pkg/exec"
"github.com/redhat-developer/odo/pkg/log"
odocontext "github.com/redhat-developer/odo/pkg/odo/context"
"github.com/redhat-developer/odo/pkg/podman"
"github.com/redhat-developer/odo/pkg/state"
Expand Down Expand Up @@ -167,5 +168,8 @@ func (o *DevClient) watchHandler(ctx context.Context, pushParams adapters.PushPa
WatchFiles: watchParams.WatchFiles,
Variables: watchParams.Variables,
}

fmt.Fprintln(watchParams.Out)
log.Fwarning(watchParams.Out, "Changes to the Devfile not supported yet on Podman. Please restart 'odo dev' for such changes to be applied.\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be displayed every time, even if the user only modifies sources files.

I would instead add an else part to the if equality.Semantic.DeepEqual(o.deployedPod, pod) condition (in the reconcile.go file), and print the warning there

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, thanks for the pointer. Initially, I had changed the part that looks for changes in the Devfile watcher (in watch.go), but then I had an issue when manually sync'ing with --no-watch.
This looks like a better approach. I'll make the change - thanks.

Copy link
Member Author

@rm3l rm3l Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One thing I just noticed: if we display the warning as part of if !equality.Semantic.DeepEqual(o.deployedPod, pod), the problem is that this would work only if the changes in the Devfile affect the resulting Pod spec.

I noticed that if I only change for example the run command in the Devfile, the resulting Pod spec is still semantically the same as the previous one, so no warning is displayed. But, the changes were not taken into account (I think because we are still using the same Devfile Object from the context).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am thinking of just re-parsing the Devfile when the watch handler is called and comparing it to the currently loaded one. And display the warning if there are any differences.
@feloy What do you think?

The Kubedev implementation actually re-parses the Devfile when the watch handler is called:

odo/pkg/dev/kubedev/kubedev.go

Lines 156 to 160 in 020009b

func (o *DevClient) regenerateComponentAdapterFromWatchParams(parameters watch.WatchParameters) (component.ComponentAdapter, error) {
devObj, err := devfile.ParseAndValidateFromFileWithVariables(location.DevfileLocation(""), parameters.Variables)
if err != nil {
return nil, err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's a good solution IMO

return o.reconcile(ctx, watchParams.Out, watchParams.ErrOut, startOptions, componentStatus)
}
62 changes: 35 additions & 27 deletions tests/integration/cmd_dev_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,45 +692,53 @@ ComponentSettings:
Expect(err).ToNot(HaveOccurred())
})

if !podman {
When("modifying memoryLimit for container in Devfile", func() {
BeforeEach(func() {
src := "memoryLimit: 1024Mi"
dst := "memoryLimit: 1023Mi"
helper.ReplaceString("devfile.yaml", src, dst)
if manual {
if os.Getenv("SKIP_KEY_PRESS") == "true" {
Skip("This is a unix-terminal specific scenario, skipping")
}

devSession.PressKey('p')
When("modifying memoryLimit for container in Devfile", func() {
var stdout string
BeforeEach(func() {
src := "memoryLimit: 1024Mi"
dst := "memoryLimit: 1023Mi"
helper.ReplaceString("devfile.yaml", src, dst)
if manual {
if os.Getenv("SKIP_KEY_PRESS") == "true" {
Skip("This is a unix-terminal specific scenario, skipping")
}
var err error
_, _, ports, err = devSession.WaitSync()
Expect(err).Should(Succeed())
})

It("should expose the endpoint on localhost", func() {
devSession.PressKey('p')
}
var err error
var stdoutBytes []byte
stdoutBytes, _, ports, err = devSession.WaitSync()
Expect(err).Should(Succeed())
stdout = string(stdoutBytes)
})

It("should expose the endpoint on localhost", func() {
rm3l marked this conversation as resolved.
Show resolved Hide resolved
if podman {
By("warning users that odo dev needs to be restarted", func() {
Expect(stdout).To(ContainSubstring(
"Changes to the Devfile not supported yet on Podman. Please restart 'odo dev' for such changes to be applied."))
})
} else {
By("updating the pod", func() {
podName := commonVar.CliRunner.GetRunningPodNameByComponent(cmpName, commonVar.Project)
bufferOutput := commonVar.CliRunner.Run("get", "pods", podName, "-o", "jsonpath='{.spec.containers[0].resources.requests.memory}'").Out.Contents()
output := string(bufferOutput)
Expect(output).To(ContainSubstring("1023Mi"))
})
}

By("exposing the endpoint", func() {
url := fmt.Sprintf("http://%s", ports["3000"])
resp, err := http.Get(url)
Expect(err).ToNot(HaveOccurred())
defer resp.Body.Close()
By("exposing the endpoint", func() {
url := fmt.Sprintf("http://%s", ports["3000"])
resp, err := http.Get(url)
Expect(err).ToNot(HaveOccurred())
defer resp.Body.Close()

body, _ := io.ReadAll(resp.Body)
helper.MatchAllInOutput(string(body), []string{"Hello from Node.js Starter Application!"})
Expect(err).ToNot(HaveOccurred())
})
body, _ := io.ReadAll(resp.Body)
helper.MatchAllInOutput(string(body), []string{"Hello from Node.js Starter Application!"})
Expect(err).ToNot(HaveOccurred())
})
})
}
})
})
})

Expand Down