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

Manual sync of files pressing p #6089

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Sep 2, 2022

What type of PR is this:

/kind feature

What does this PR do / why we need it:

This PR configures the terminal to work in Character mode, instead of the default Line mode. Read() will return as soon as a character is sent to the input stream, instead of waiting for a newline character.

code refactoring:

  • moved getFullSourcesWatcher and related functions to a specific file file_watcher.go
  • moved long list of watchers variables into receiver instead of passing as parameters

Which issue(s) this PR fixes:

Fixes #5634

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

Refactoring integration tests

Some tests were calling devSession.Kill() (or Stop()) twice, which makes ginkgo stop abrupty during the second call on Windows.

In the following example, the Kill in AfterEach is necessary for the cleanup after test1, but is problematic for test2, as Stop has been called before.

As the AfterEach/BeforeEach are evaluated at the beginning, it is not possible to rely on a field in devSession to record if the method has already been called.

When("odo dev is executed", func() {

			var devSession helper.DevSession

			BeforeEach(func() {
				var err error
				devSession, _, _, _, err = helper.StartDevMode(nil)
				Expect(err).ToNot(HaveOccurred())
			})

			AfterEach(func() {
				devSession.Kill()
				devSession.WaitEnd()
			})

			It("test 1", func() { ... })

			When("odo dev is stopped", func() {
				BeforeEach(func() {
					devSession.Stop()
					devSession.WaitEnd()
				})

				It("test 2", func() { ... })
			})
		})

These tests have been split in two trees, separating test1 and test2, to be able to call Kill/Stop only once.

@netlify
Copy link

netlify bot commented Sep 2, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 406923f
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/631b4d961766d50009374924
😎 Deploy Preview https://deploy-preview-6089--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Sep 2, 2022
@openshift-ci openshift-ci bot requested review from cdrage and dharmit September 2, 2022 12:13
@feloy feloy changed the title Manual sync of files pressing p [WIP] Manual sync of files pressing p Sep 2, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Sep 2, 2022
@odo-robot
Copy link

odo-robot bot commented Sep 2, 2022

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

@odo-robot
Copy link

odo-robot bot commented Sep 2, 2022

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

@feloy feloy force-pushed the feature-5634/manual-sync-2 branch from 7295473 to 39b10e1 Compare September 2, 2022 12:30
@odo-robot
Copy link

odo-robot bot commented Sep 2, 2022

OpenShift Tests on commit 978f45b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 2, 2022

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

@feloy
Copy link
Contributor Author

feloy commented Sep 5, 2022

Blocked by #6091

@feloy feloy added the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Sep 5, 2022
@feloy feloy mentioned this pull request Sep 5, 2022
3 tasks
@valaparthvi valaparthvi removed the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Sep 6, 2022
@feloy feloy force-pushed the feature-5634/manual-sync-2 branch from ce9abac to 6462d62 Compare September 7, 2022 07:51
@odo-robot
Copy link

odo-robot bot commented Sep 7, 2022

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

@feloy feloy force-pushed the feature-5634/manual-sync-2 branch 3 times, most recently from 25d9a3f to 4feaaf8 Compare September 7, 2022 14:37
@feloy feloy changed the title [WIP] Manual sync of files pressing p Manual sync of files pressing p Sep 7, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Sep 7, 2022
@feloy feloy closed this Sep 7, 2022
@feloy feloy reopened this Sep 7, 2022
@feloy feloy requested a review from rm3l September 8, 2022 09:10
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. Added a few code-related comments I had so far.

@rm3l rm3l self-assigned this Sep 8, 2022
@feloy feloy force-pushed the feature-5634/manual-sync-2 branch from 7af8493 to 8f39f15 Compare September 8, 2022 11:10
@feloy
Copy link
Contributor Author

feloy commented Sep 8, 2022

Thanks @rm3l for your review

@feloy feloy requested a review from rm3l September 8, 2022 11:11
@feloy
Copy link
Contributor Author

feloy commented Sep 9, 2022

/hold Windows tests are failing on CI (non required)

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. and removed lgtm Indicates that a PR is ready to be merged. Required by Prow. labels Sep 9, 2022
@feloy feloy force-pushed the feature-5634/manual-sync-2 branch 2 times, most recently from 42f7717 to a6cfb2e Compare September 9, 2022 13:33
@feloy feloy force-pushed the feature-5634/manual-sync-2 branch from 0b45a78 to 406923f Compare September 9, 2022 14:28
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@feloy feloy removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 9, 2022
@feloy
Copy link
Contributor Author

feloy commented Sep 9, 2022

@anandrkskd I've added a PR to fix the problem of failing integration tests on Windows

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 9, 2022
@feloy
Copy link
Contributor Author

feloy commented Sep 9, 2022

/override ci/prow/v4.10-integration-e2e
Tests pass on IBM cloud

@openshift-ci
Copy link

openshift-ci bot commented Sep 9, 2022

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e
Tests pass on IBM cloud

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.

@openshift-merge-robot openshift-merge-robot merged commit 59f4f83 into redhat-developer:main Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manually trigger file sync in odo dev
5 participants