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

rm3l · 2023-01-09T08:31:23Z

What type of PR is this:
/area odo-on-podman

What does this PR do / why we need it:

Which issue(s) this PR fixes:
Fixes #6336

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

… when local changes are detected The initial idea was to display such a warning only when changes are detected in the Devfile, which are not yet handled completely at this time. But to take into account the manual synchronization case (where there is no file watcher (and therefore no way to determine which files have actually been modified)), a generic warning is being displayed whenever the Podman-specific watch handler is called.

netlify · 2023-01-09T08:31:31Z

✅ Deploy Preview for odo-docusaurus-preview ready!

Name	Link
🔨 Latest commit	`82ace2f`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/63bd7d251908c30008e6ee8f
😎 Deploy Preview	https://deploy-preview-6477--odo-docusaurus-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

odo-robot · 2023-01-09T08:37:40Z

OpenShift Tests on commit e2ef2d8 finished with errors.
View logs: TXT HTML

odo-robot · 2023-01-09T08:37:58Z

NoCluster Tests on commit e2ef2d8 finished successfully.
View logs: TXT HTML

odo-robot · 2023-01-09T08:39:04Z

Unit Tests on commit e2ef2d8 finished successfully.
View logs: TXT HTML

odo-robot · 2023-01-09T08:48:10Z

Validate Tests on commit e2ef2d8 finished successfully.
View logs: TXT HTML

odo-robot · 2023-01-09T08:51:56Z

Kubernetes Tests on commit e2ef2d8 finished successfully.
View logs: TXT HTML

odo-robot · 2023-01-09T08:53:12Z

Windows Tests (OCP) on commit e2ef2d8 finished successfully.
View logs: TXT HTML

feloy · 2023-01-09T12:30:42Z

pkg/dev/podmandev/podmandev.go

@@ -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")


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

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.

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).

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

}

Yes, that's a good solution IMO

…started when local changes are detected Display a warning that "odo dev" on Podman might need to be restarted when the Devfile is modified

odo-robot · 2023-01-09T18:07:44Z

OpenShift Tests on commit 3023bb8 finished successfully.
View logs: TXT HTML

feloy

Also, could you add a test to check that no warning is displayed when the Devfile is not changed?
As the code to read the original and new Devfiles is not in the same place, I'm afraid it could drift over time

tests/integration/cmd_dev_test.go

sonarcloud · 2023-01-10T15:01:34Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

feloy · 2023-01-10T20:39:19Z

/override OpenShift-Integration-tests/OpenShift-Integration-tests
Flaky e2e test

openshift-ci · 2023-01-10T20:39:23Z

@feloy: Overrode contexts on behalf of feloy: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

/override OpenShift-Integration-tests/OpenShift-Integration-tests
Flaky e2e test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rm3l added 2 commits January 9, 2023 09:05

Add integration test case

d45cfbd

openshift-ci bot added the area/odo-on-podman Issues or PRs related to running odo against Podman label Jan 9, 2023

openshift-ci bot requested review from feloy and valaparthvi January 9, 2023 08:31

rm3l added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Jan 9, 2023

feloy reviewed Jan 9, 2023

View reviewed changes

rm3l added 2 commits January 9, 2023 18:30

fixup! Add integration test case

7b71a78

amend! Display a warning that "odo dev" on Podman might need to be re…

6f51b75

…started when local changes are detected Display a warning that "odo dev" on Podman might need to be restarted when the Devfile is modified

rm3l changed the title ~~Display a warning that "odo dev" on Podman might need to be restarted if the Devfile is changed~~ Display a warning that "odo dev" on Podman needs to be restarted if the Devfile is changed Jan 9, 2023

rm3l closed this Jan 9, 2023

rm3l reopened this Jan 9, 2023

feloy reviewed Jan 10, 2023

View reviewed changes

tests/integration/cmd_dev_test.go Outdated Show resolved Hide resolved

rm3l added 2 commits January 10, 2023 15:44

fixup! Add integration test case

8058649

Assert that no warning is displayed if the Devfile has not changed

82ace2f

rm3l requested a review from feloy January 10, 2023 16:36

feloy approved these changes Jan 10, 2023

View reviewed changes

openshift-ci bot assigned feloy Jan 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jan 10, 2023

openshift-merge-robot merged commit da9a9ef into redhat-developer:main Jan 10, 2023

rm3l deleted the 6336-warn-users-that-odo-dev-needs-to-be-restarted-when-related-information-is-changed-in-devfile branch January 10, 2023 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rm3l commented Jan 9, 2023

netlify bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

feloy Jan 9, 2023

rm3l Jan 9, 2023

rm3l Jan 9, 2023 •

edited

Loading

rm3l Jan 9, 2023

feloy Jan 9, 2023

odo-robot bot commented Jan 9, 2023

feloy left a comment

sonarcloud bot commented Jan 10, 2023

feloy commented Jan 10, 2023

openshift-ci bot commented Jan 10, 2023

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

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

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

Conversation

rm3l commented Jan 9, 2023

netlify bot commented Jan 9, 2023 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview ready!

odo-robot bot commented Jan 9, 2023 • edited Loading

odo-robot bot commented Jan 9, 2023 • edited Loading

odo-robot bot commented Jan 9, 2023 • edited Loading

odo-robot bot commented Jan 9, 2023 • edited Loading

odo-robot bot commented Jan 9, 2023 • edited Loading

odo-robot bot commented Jan 9, 2023 • edited Loading

feloy Jan 9, 2023

Choose a reason for hiding this comment

rm3l Jan 9, 2023

Choose a reason for hiding this comment

rm3l Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

rm3l Jan 9, 2023

Choose a reason for hiding this comment

feloy Jan 9, 2023

Choose a reason for hiding this comment

odo-robot bot commented Jan 9, 2023

feloy left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 10, 2023

feloy commented Jan 10, 2023

openshift-ci bot commented Jan 10, 2023

netlify bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

odo-robot bot commented Jan 9, 2023 •

edited

Loading

rm3l Jan 9, 2023 •

edited

Loading