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

Add auto-detect when no config file is given #52

Merged
merged 2 commits into from
Sep 9, 2020

Conversation

mehdy
Copy link
Collaborator

@mehdy mehdy commented Sep 3, 2020

Closes #50

Copy link
Owner

@preslavmihaylov preslavmihaylov left a comment

Choose a reason for hiding this comment

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

Could you also add a follow-up task to document this behaviour in the README \w a link to the PR/issue?

testing/scenarios/auto_detect_config/expected_config.yaml Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Show resolved Hide resolved
config/config.go Show resolved Hide resolved
@preslavmihaylov
Copy link
Owner

Blocked by #54

@mehdy
Copy link
Collaborator Author

mehdy commented Sep 6, 2020

@preslavmihaylov Made some changes. Could you please re-review it?

}
if err := ioutil.WriteFile(gitDir+"/config", []byte(gitConfig), 0644); err != nil {
t.Errorf("%s", err)
}
Copy link
Owner

Choose a reason for hiding this comment

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

This is a nice way to handle the issue.

Could you extract it in the scenariobuilder as e.g. WithGitConfig(the parameters added in a git config) so that it can be reused across several tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

// make sure it's there beforehand.
gitDir := "./scenarios/auto_detect_config/.git"
gitConfig := `[remote "origin"]
url = git@github.com:test_username/test_repo.git
Copy link
Owner

Choose a reason for hiding this comment

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

Could you also add a test for detecting an origin specified without the .git extension? E.g.

	url = https://github.com/preslavmihaylov/todocheck

Copy link
Owner

Choose a reason for hiding this comment

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

This is why I requested the change about WithGitConfig

Copy link
Owner

Choose a reason for hiding this comment

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

Full configuration example:

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
	ignorecase = true
	precomposeunicode = true
[remote "origin"]
	url = https://github.com/preslavmihaylov/todocheck
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = origin
	merge = refs/heads/master
	rebase = true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@preslavmihaylov preslavmihaylov self-requested a review September 7, 2020 06:14
preslavmihaylov
preslavmihaylov previously approved these changes Sep 7, 2020
@preslavmihaylov preslavmihaylov self-requested a review September 7, 2020 06:15
@preslavmihaylov preslavmihaylov dismissed their stale review September 7, 2020 06:16

Additional changes are requested

Copy link
Owner

@preslavmihaylov preslavmihaylov left a comment

Choose a reason for hiding this comment

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

There is still one more test case I'd like us to set in place in order to make sure we cover deriving the origin from both ssh-based URIs and http-based ones.

Additionally, the request for documenting the new behavior in the README or creating a new issue for it still stands.

Signed-off-by: Mehdy Khoshnoody <mehdy.khoshnoody@gmail.com>
Signed-off-by: Mehdy Khoshnoody <mehdy.khoshnoody@gmail.com>
@mehdy
Copy link
Collaborator Author

mehdy commented Sep 8, 2020

Created a new issue to add docs about this feature. #58

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.

Auto detect issue tracker based on git remote address
2 participants