Skip to content

test: remove git initializer TargetMode and CloneTaget #13277

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
Merged
merged 1 commit into from
Sep 26, 2022
Merged

test: remove git initializer TargetMode and CloneTaget #13277

merged 1 commit into from
Sep 26, 2022

Conversation

jenting
Copy link
Contributor

@jenting jenting commented Sep 26, 2022 •
edited
Loading

Description

Remove git initializer TargetMode and CloneTaget in the workspace integration test.

Related Issue(s)

Fixes #13278

How to test

Running the integration test, make sure the integration test pass.

Release Notes

None

Documentation

None

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=workspace
    Valid options are all, workspace, webapp, ide

@jenting jenting requested a review from a team September 26, 2022 03:39
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Sep 26, 2022
@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022

/hold

Waits for the integration test pass

@kylos101
Copy link
Contributor

kylos101 commented Sep 26, 2022 •
edited
Loading

👋 @jenting can you share context for this PR? For example, the issue you linked to is closed. Is there a new issue or problem that was reported?

edit: I assume you are referring to this comment? #13241 (comment)

@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022 •
edited
Loading

👋 @jenting can you share context for this PR? For example, the issue you linked to is closed. Is there a new issue or problem that was reported?

edit: I assume you are referring to this comment? #13241 (comment)

Yes, because we could fix it within 5 mins, so I did not bring up the issue originally.
I just created the issue and updated this PR description.

@jenting jenting marked this pull request as draft September 26, 2022 04:03
@jenting jenting force-pushed the jenting/revert-git-ls-remote-exit-code branch from 441b3b8 to e80b11a Compare September 26, 2022 04:03
@roboquat roboquat added size/S and removed size/XS labels Sep 26, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-jenting-revert-git-ls-remote-exit-code.2 because the annotations in the pull request description changed
(with .werft/ from main)

@jenting jenting force-pushed the jenting/revert-git-ls-remote-exit-code branch from e80b11a to 6e7b926 Compare September 26, 2022 04:12
@roboquat roboquat added size/M and removed size/S labels Sep 26, 2022
@jenting jenting force-pushed the jenting/revert-git-ls-remote-exit-code branch from 6e7b926 to 3c88028 Compare September 26, 2022 04:39
@roboquat roboquat added size/S and removed size/M labels Sep 26, 2022
@jenting jenting changed the title content-service: revert git ls-remote with --exit-code flag test: remove git initializer TargetMode and CloneTaget Sep 26, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-jenting-revert-git-ls-remote-exit-code.5 because the annotations in the pull request description changed
(with .werft/ from main)

@jenting jenting force-pushed the jenting/revert-git-ls-remote-exit-code branch from 3c88028 to f193c27 Compare September 26, 2022 05:32
@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022 •
edited
Loading

This PR is blocked by issue #13284.

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-jenting-revert-git-ls-remote-exit-code.7 because the annotations in the pull request description changed
(with .werft/ from main)

@jenting jenting marked this pull request as ready for review September 26, 2022 07:07
@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-jenting-revert-git-ls-remote-exit-code.8
(with .werft/ from main)

@utam0k
Copy link
Contributor

utam0k commented Sep 26, 2022

I have one question. Should this modify the test, or is this something that contentInitializer should handle (or ignore)?

@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022

I have one question. Should this modify the test, or is this something that contentInitializer should handle (or ignore)?

I use Jaeger to catch the initializer sent from the server, and it does not contain the TargetMode and CloneTarget from the start workspace request. That's why we can't reproduce it when we open the workspace from Gitpod GUI.

For the short term, we could fix it with an integration test.

But I am thinking about the long term. We should change the workspace integration test that sends requests from integration test client -> server -> ws-manager, rather than from integration test client-> ws-manager.
After that, we could test against the real code logic, WDYT?

@utam0k
Copy link
Contributor

utam0k commented Sep 26, 2022

For the short term, we could fix it with an integration test.

But I am thinking about the long term. We should change the workspace integration test that sends requests from integration test client -> server -> ws-manager, rather than from integration test client-> ws-manager.
After that, we could test against the real code logic, WDYT?

Make sense 💯 Please put TODO comment about it to the integration test.

@jenting jenting force-pushed the jenting/revert-git-ls-remote-exit-code branch from f193c27 to 228515c Compare September 26, 2022 07:42
@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-jenting-revert-git-ls-remote-exit-code.10
(with .werft/ from main)

@jenting
Copy link
Contributor Author

jenting commented Sep 26, 2022

/unhold

@roboquat roboquat merged commit 988d75e into main Sep 26, 2022
@roboquat roboquat deleted the jenting/revert-git-ls-remote-exit-code branch September 26, 2022 08:32
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utam0k utam0k utam0k approved these changes

Assignees
No one assigned
Labels
deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note-none size/S team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

workspace integration test failed with git ls-remote --exit-code origin main failed (exit status 2)
4 participants
@jenting @kylos101 @utam0k @roboquat