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) Introduces a "local" SCM with autoguessing #713

Merged
merged 2 commits into from
Jul 4, 2022

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Jun 7, 2022

Ref. #716

Depends on #717

This change request introduces a new feature for Updatecli: an automatic SCM identified as local.

The "automatic guess" feature is not complete: for now, it only extracts some of the atrributes, the others have to be specified through manifest or env. vars. I expect more PRs to improve it along the way.
That's why the associated issue won't be closed.

For now:

  • URL for git
  • Owner/repository for github

Please note that:

  • A new go module dependency was added: github.com/goware/urlx. Not strictly necessarly, but really useful to parse the hostname of the github SSH addresses. Might be doable with a good regexp, but that was too distracting for me to write such (as it needs tests, etc.).
  • The gitgeneric package now provides an interface, implemented by the default GoGit{} struct. Should not change anything (plugins git, github and others have been updated) but it allows mocking (a basic mock is provided because used by the code introduced in this PR) and would help in unit testing any SCM-dependent behavior.
  • The config package was split (errors.go to store the errors, defaults.go for the logic around setting up default configuration). Be careful with rebases!
  • The scm package was split to have a clear source file separation between scm.Scm behaviors and scm.Config behaviors (Validate was confusing before)
  • There are 2 Merge methods for git and github SCM plugins. This code could definitivelt be improved in the future, but it's around the whole "configuration definition" + "deep merge tree structures with zero-ed values". Too much compared to writing the 2 raws methods. Can be improved, but since it's tested and since we do not define new SCM plugins everyday, it's worth it for me.
  • Unit tests, unit tests and unit tests
  • The end to end manifests where modified to disable the local SCM as none of them use it (to avoid unplanned info/warning message).

Test

To test this pull request, you can run the following commands:

make test
make test-e2e

Additional Information

  • The end to end tests where modified to make its easier to run locally.
    • A change has been introduced in the Makefile to explain the env. var $VENOM_VAR_binpath but also to fail fast with a comprehensive message when missing env vars are absent (instead of starting venom which fails)
    • We can still improve the contributor experience for E2E but later (splitting tests for easier "catch which onefailed") :)
    • No more values.yaml for now. If we want to end-to-end testing of the templating of values, we have to add a specific test for this. Otherwise it adds too much varying elements. Another idea: we can also make this file back, but the Makefile would parse it to list the required environment variables in a centralized location).

pkg/core/config/main.go Outdated Show resolved Hide resolved
@dduportal dduportal force-pushed the feat/scm-local branch 7 times, most recently from d7c3b15 to b79c139 Compare June 12, 2022 20:13
@dduportal dduportal changed the title Wip: introduces a "local" SCM feat(SCM) Introduces a "local" SCM with autoguessing Jun 13, 2022
@dduportal dduportal requested a review from olblak June 13, 2022 07:29
@dduportal dduportal marked this pull request as ready for review June 13, 2022 10:25
@dduportal dduportal added the enhancement New feature or request label Jun 13, 2022
@olblak
Copy link
Member

olblak commented Jun 14, 2022

Overall, it's a great PR. Thanks for paving the way
Considering the size of the work needed here, I totally support merging this PR even if it doesn't solve the global issue.

But before merging I would like to clarify one thing

Disable local scm

I don't understand why and when to use the following block

scms:
  local:
    disabled: true

lemeurherve
lemeurherve previously approved these changes Jun 14, 2022
Copy link
Member

@lemeurherve lemeurherve left a comment

Choose a reason for hiding this comment

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

Minor comment nitpicks

Great job! 🎉

e2e/values.yaml Outdated Show resolved Hide resolved
pkg/core/config/defaults.go Outdated Show resolved Hide resolved
pkg/core/config/errors.go Outdated Show resolved Hide resolved
pkg/core/config/errors.go Outdated Show resolved Hide resolved
pkg/core/config/errors.go Outdated Show resolved Hide resolved
pkg/core/config/main.go Outdated Show resolved Hide resolved
pkg/core/pipeline/scm/config.go Outdated Show resolved Hide resolved
pkg/core/pipeline/scm/main.go Outdated Show resolved Hide resolved
pkg/plugins/utils/gitgeneric/main_test.go Outdated Show resolved Hide resolved
pkg/plugins/utils/gitgeneric/mocks.go Outdated Show resolved Hide resolved
lemeurherve
lemeurherve previously approved these changes Jun 14, 2022
@olblak olblak modified the milestone: 0.27.0 Jun 25, 2022
…mentation + mock implementation and add a new method RemoteUrls

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

Rebase in progress (with conflict merge)

…etection

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Co-authored-by: Hervé Le Meur <91831478+lemeurherve@users.noreply.github.com>
@dduportal
Copy link
Contributor Author

@olblak Ready to review (approve/merge if ok).

I've done full testing on jenkins-infra/kubernetes-management as usual with a updatecli diff with this PR.
Also tested the full "apply" on https://github.com/dduportal/docker-hashicorp-tools/pulls (3 opened PRs).

Copy link
Member

@olblak olblak left a comment

Choose a reason for hiding this comment

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

🎉

@olblak olblak merged commit eb44195 into updatecli:main Jul 4, 2022
@olblak olblak added this to the 0.28.0 milestone Jul 8, 2022
@dduportal dduportal deleted the feat/scm-local branch November 19, 2022 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants