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

feat(scm): add new native scm driver #542

Closed
wants to merge 14 commits into from
Closed

feat(scm): add new native scm driver #542

wants to merge 14 commits into from

Conversation

kneal
Copy link
Contributor

@kneal kneal commented Nov 26, 2021

PR includes the following changes:

  • add a Bitbucket option for using the native driver
  • add a native driver implentation based on the jenkins-x/go-scm library

closes go-vela/community#441

Notes:

  • The SCM package has been functionally tested with a set of pipelines against GitHub and Bitbucket.
  • I will not be adding docs yet because I do not recommend using the package until further testing is completed
  • The new library has a lot of testing downsides I would like to re-think in next pr. (Documented with TODOs)

@kneal kneal added the feature Indicates a new feature label Nov 26, 2021
@kneal kneal self-assigned this Nov 26, 2021
@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #542 (0b7c390) into master (74b5d14) will decrease coverage by 1.47%.
The diff coverage is 34.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   54.85%   53.38%   -1.48%     
==========================================
  Files         179      188       +9     
  Lines       14899    16073    +1174     
==========================================
+ Hits         8173     8580     +407     
- Misses       6409     7149     +740     
- Partials      317      344      +27     
Impacted Files Coverage Δ
scm/native/changeset.go 0.00% <0.00%> (ø)
scm/native/webhook.go 0.00% <0.00%> (ø)
scm/native/access.go 22.42% <22.42%> (ø)
scm/native/repo.go 23.79% <23.79%> (ø)
scm/native/deployment.go 38.75% <38.75%> (ø)
scm/setup.go 79.22% <42.85%> (-20.78%) ⬇️
scm/native/authentication.go 67.12% <67.12%> (ø)
scm/native/opts.go 88.67% <88.67%> (ø)
scm/native/native.go 95.91% <95.91%> (ø)
scm/native/driver.go 100.00% <100.00%> (ø)

@cognifloyd
Copy link
Member

Why "native"?
Other possibilities: "generic", "abstract", "shim"

@kneal
Copy link
Contributor Author

kneal commented Nov 29, 2021

I was originally thinking generic for the package name but we actually already use the native nomenclature in a few package interfaces. The thought was native might sync up with that thought better and do like the standard OAuth esque integrations and then other packages could focus on being more 1st class citizens to the individual SCMs if we want that.

Definitely not stuck on the name by any means, I'm largely indifferent on what we call it. I just thought in the moment it made sense when I was first structuring out the package/files.

@kneal kneal marked this pull request as ready for review January 3, 2022 14:24
@kneal kneal requested a review from a team as a code owner January 3, 2022 14:24
@kneal kneal marked this pull request as ready for review January 7, 2022 15:59
@@ -0,0 +1,164 @@
// Copyright (c) 2021 Target Brands, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we update the copyright statement to use the current year? 👍

I think this change will need to be applied throughout the files being added here.


// OrgAccess captures the user's access level for an org.
func (c *client) OrgAccess(u *library.User, org string) (string, error) {
logrus.Tracef("Capturing %s access level to org %s", u.GetName(), org)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we update the logging statements to reflect current state in the codebase? 👍

i.e.

// OrgAccess captures the user's access level for an org.
func (c *client) OrgAccess(u *library.User, org string) (string, error) {
c.Logger.WithFields(logrus.Fields{
"org": org,
"user": u.GetName(),
}).Tracef("capturing %s access level to org %s", u.GetName(), org)

I think this change will need to be applied throughout the functions and files being added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by "current state" in this context? Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

By current state, I mean what we currently have in the SCM codebase on the master branch 👍

That link I shared above should show what I'm talking about 😃

To clarify further with an example, I'd expect this line to be updated to:

Suggested change
logrus.Tracef("Capturing %s access level to org %s", u.GetName(), org)
c.Logger.WithFields(logrus.Fields{
"org": org,
"user": u.GetName(),
}).Tracef("capturing %s access level to org %s", u.GetName(), org)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I'm sorry I see it now. You mean using the client logger. Sorry, for some reason my eyes weren't picking up the difference. Yeah, super easy change!

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to apologize 😄 It's all good!

Hopefully you should be able to copy/paste almost all the logging statements from the current SCM code 👍

However, it may be tedious because there are a lot of them 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better to get it out of the way now tho. So good call out!

@KellyMerrick
Copy link
Contributor

@kneal looks like this has gone stale, so we're going to close for now, but are interested in adding more SCM providers. Please reopen if we want to move forward with it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new generic scm type to work with a larger set of scms
5 participants