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 integration for Coding.net #2028

Merged
merged 1 commit into from Jul 24, 2017
Merged

Add integration for Coding.net #2028

merged 1 commit into from Jul 24, 2017

Conversation

mingshun
Copy link

@mingshun mingshun commented May 8, 2017

This PR is for adding integration for Coding.net including platform and enterprise edition.

@CLAassistant
Copy link

CLAassistant commented May 8, 2017

CLA assistant check
All committers have signed the CLA.

@vangie
Copy link

vangie commented May 8, 2017

@fly88oj
Copy link

fly88oj commented May 8, 2017

+1

@bradrydzewski
Copy link

thanks! I will start the review process and let you know if I have any comments. Great work!

@bradrydzewski bradrydzewski added this to To Do in Version 0.7 May 11, 2017
@bradrydzewski bradrydzewski moved this from To Do to In Progress in Version 0.7 May 11, 2017
@bradrydzewski
Copy link

cc @appleboy are you able to help review and push this through? Thanks! :)

@bradrydzewski
Copy link

@mingshun can you also update the conflict? thanks!

@bradrydzewski bradrydzewski removed this from In Progress in Version 0.7 May 24, 2017
@bradrydzewski bradrydzewski added this to In Progress in Version 0.8 May 24, 2017
@appleboy
Copy link

@bradrydzewski I will test this PR after @mingshun fix conflicts.

@mingshun
Copy link
Author

@appleboy conflicts fixed.

@bradrydzewski bradrydzewski moved this from In Progress to To Do in Version 0.8 Jun 30, 2017
@bradrydzewski bradrydzewski removed this from To Do in Version 0.8 Jul 16, 2017
@bradrydzewski
Copy link

thanks! perhaps we can get this included in 0.8-rc.2

@bradrydzewski
Copy link

@mingshun as we prepare to review and get this merged, would you mind adding setup instructions to https://github.com/drone/docs/tree/master/content/install/integrations ? They should look similar to http://docs.drone.io/install-for-bitbucket-cloud/

thanks! :)

@appleboy
Copy link

appleboy commented Jul 23, 2017

LGTM. I already tested with https://coding.net/u/appleboy/p/go-hello/git repo. It is working for me. cc @bradrydzewski

Copy link

@bradrydzewski bradrydzewski left a comment

Choose a reason for hiding this comment

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

just one minor change requested, but otherwise looks great :)

},
cli.StringFlag{
EnvVar: "DRONE_CODING_GIT_MACHINE",
Name: "coding-git-machine",

Choose a reason for hiding this comment

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

we should be able to derive the hostname from the DRONE_CODING_URL value. See https://github.com/drone/drone/blob/master/remote/gogs/gogs.go#L41:L47 for an example

if opts.URL != defaultURL {
remote.URL = strings.TrimSuffix(opts.URL, "/")
}

Choose a reason for hiding this comment

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

we should be able to do something like this (which I mentioned in my prior comment) so that we can remove the DRONE_CODING_MACHINE environment variable.

const (
	defaultURL  = "https://coding.net" // Default Coding URL
+	defaultHost = "coding.net" // Default Coding Host
)


func New(opts Opts) (remote.Remote, error) {
	remote := &Coding{
		URL:        defaultURL,
		Client:     opts.Client,
		Secret:     opts.Secret,
		Scopes:     opts.Scopes,
-		Machine:    opts.Machine,
+		Machine:    defaultHost,
		Username:   opts.Username,
		Password:   opts.Password,
		SkipVerify: opts.SkipVerify,
	}
	if opts.URL != defaultURL {
		remote.URL = strings.TrimSuffix(opts.URL, "/")
+		url, err := url.Parse(remote.URL)
+		if err != nil {
+			return nil, err
+		}
+		host, _, err := net.SplitHostPort(url.Host)
+		if err == nil {
+			url.Host = host
+		}
+		remote.Machine  = host
	}

	// Hack to enable oauth2 access in coding's implementation
	oauth2.RegisterBrokenAuthHeaderProvider(remote.URL)
	return remote, nil
}

Copy link
Author

Choose a reason for hiding this comment

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

The API hostname and Git hostname are different hostnames in Coding. So I have to keep the DRONE_CODING_GIT_MACHINE.

Copy link

Choose a reason for hiding this comment

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

@mingshun can we use a default value for the machine? Is it safe to assume that if most users configure drone with the default url, they will also be able to use a default git host?

Copy link
Author

Choose a reason for hiding this comment

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

@bradrydzewski But we have enterprise edition using another hostname. And we have self-host edition which may have even different hostnames.

Choose a reason for hiding this comment

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

ok, understood. I didn't realize coding.net could be self hosted as well :)

@bradrydzewski
Copy link

ok LGTM

now we just need some docs added to github.com/drone/docs

@bradrydzewski bradrydzewski merged commit 6930274 into harness:master Jul 24, 2017
@mingshun
Copy link
Author

@bradrydzewski added docs to drone/docs#228

@bradrydzewski bradrydzewski added this to the v1.0.0 milestone Jul 24, 2017
@bradrydzewski bradrydzewski added this to Done in Version 0.8 Jul 24, 2017
@Youngv
Copy link

Youngv commented Mar 13, 2018

@mingshun Any update for Coding.net with the 0.8 version? It's not working now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants