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

[cli] replace pstree in credential helper #4385

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

JanKoehnlein
Copy link
Contributor

Fixes #4062

@JanKoehnlein
Copy link
Contributor Author

JanKoehnlein commented Jun 3, 2021

/werft run

@JanKoehnlein
Copy link
Contributor Author

JanKoehnlein commented Jun 3, 2021

/werft run

👍 started the job as gitpod-build-jk-gitpod-extension-warning-4062.1

@JanKoehnlein JanKoehnlein marked this pull request as ready for review June 3, 2021 10:13
@AlexTugarev
Copy link
Member

AlexTugarev commented Jun 8, 2021

/werft run

👍 started the job as gitpod-build-jk-gitpod-extension-warning-4062.4

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Quickly checked with the repo from GH issue and it works as advertised!

The remote repository "https://github.com/icub-tech-iit/documentation.git" is not accessible with the current token. Please grant the necessary permissions.

@corneliusludmann
Copy link
Contributor

@JanKoehnlein Is it still work in process? Asking due to the [wip] prefix. It's a left-over, right?

@JanKoehnlein JanKoehnlein changed the title [wip] replace pstree in credential helper [cli] replace pstree in credential helper Jun 8, 2021
@JanKoehnlein
Copy link
Contributor Author

Changed the title to reflect that this is ready for review

func logDebug(v ...interface{}) {
if os.Getenv("CREDENTIAL_HELPER_DEBUG_LOG") == "true" {
log.Println(v...)
func parseProcessTree() (repoUrl string, gitCommand string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we could write some unit tests for this? Not just for the actual test but that would also help to understand the function because we would have a list of some example inputs and expected outputs.

Sure, the problem here is to mock the readProc function for the test, right? Any ideas?

Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

Code looks good so far. Haven't tested since Alex has already.

The proposed unit test would be great, though. But since it wasn't there before either, it doesn't get any worse with this change. Therefore no blocker.

@JanKoehnlein
Copy link
Contributor Author

The crucial part is to interpret/parse what comes out of /proc in a very special scenario, which is hard to reproduce in a unit test. I don't see that there is more to consider beyond the happy path that is covered with manual testing, so I think we're fine.

@JanKoehnlein JanKoehnlein merged commit 4b8a4ef into main Jun 8, 2021
@JanKoehnlein JanKoehnlein deleted the jk-gitpod-extension-warning-4062 branch June 8, 2021 14:59
JanKoehnlein added a commit that referenced this pull request Jun 9, 2021
MatthewFagan pushed a commit to trilogy-group/gitpod that referenced this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitpod Extension warning that I have to grant necessary permissions
3 participants