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

Support Git as only VCS #4346

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

qwerty287
Copy link
Contributor

Only support git as vcs and remove the corresponding env var CI_REPO_SCM.

@qwerty287 qwerty287 added refactor delete or replace old code breaking will break existing installations if no manual action happens labels Nov 10, 2024
@qwerty287 qwerty287 requested a review from a team November 10, 2024 15:58
anbraten
anbraten previously approved these changes Nov 10, 2024
docs/src/pages/migrations.md Outdated Show resolved Hide resolved
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

e.g. #2355 integration

and more could come, so it does not add much maintinance to keep it as generic as it is now

@6543
Copy link
Member

6543 commented Nov 10, 2024

why was this initially proposed at all? e.g. what's the reason behind?

@6543
Copy link
Member

6543 commented Nov 10, 2024

I see the current architecture that we can support more than git even as far as an "selling" over other cicd systems

@xoxys
Copy link
Member

xoxys commented Nov 10, 2024

e.g. #2355 integration

How are other git forges that were implemented by an external plugin related to removing none-git vsc systems?

I see the current architecture that we can support more than git even as far as an "selling" over other cicd systems

I don't see it as a selling point especially not as e can't even handle all existing bugs for the existing feature set already.

why was this initially proposed at all?!? e.g. what's the reason behind?

I would like to kindly ask you again not to communicate with so many punctuation marks. It's hard to read and a somewhat “aggressive” tone.

@6543
Copy link
Member

6543 commented Nov 10, 2024

PS: still thanks for thinking how to slim down and remove legacy code :)

@qwerty287
Copy link
Contributor Author

qwerty287 commented Nov 11, 2024

I don't see it as a selling point especially not as e can't even handle all existing bugs for the existing feature set already.

That's also my point. Radicle has a git "bridge" so it should work.

And we can't have everything as selling point. At some point, we should have some limitations, we can't build the "egg-laying woolly milk sow" (don't know if there's a proper English saying 😅).

@6543
Copy link
Member

6543 commented Nov 11, 2024

we can't build the "egg-laying woolly milk sow"

agree but this is realy not much code and losing it is a shame ...

@6543
Copy link
Member

6543 commented Nov 11, 2024

I as an owner wana veto this change

@pat-s
Copy link
Contributor

pat-s commented Nov 15, 2024

Without any opinion: Should this PR be closed then?

@xoxys
Copy link
Member

xoxys commented Nov 15, 2024

If a single owner has the power to block PRs at any time, yes.

@qwerty287
Copy link
Contributor Author

Well, @anbraten approved it...

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Nov 16, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-4346.surge.sh

@6543
Copy link
Member

6543 commented Nov 17, 2024

well the argument stands: either merge it, in this case I will also merge things, that got denyed from me, because an owner is for it ... or block it untill veto is removed in this case untill I'm no owner anymore. next election is in 1 months btw. ...

so yes our community gudelines dont have a clear statement of when the owners dissagree - wich would not be a problem if we would have $owner.mod(2) = 0 (and if there is something stated as majority of owners agree ..), .. witch we currently dont.

so I personally interpret it as veto. ?

@xoxys
Copy link
Member

xoxys commented Nov 17, 2024

I just dont understand your point. Right now there is not a single none-git forge supported in woodpecker core right? So this code is unused and useless. To keep it just because we might or might not support none-git forges doesnt make sense to me. Even if we would like to support such forges we could also do it by an official extension in the same way we do it for plugins.

@pat-s
Copy link
Contributor

pat-s commented Nov 18, 2024

As this is now more a political issue than a code-one, it would be great if you @6543 @anbraten could discuss the general governance part how to proceed in such situations.

@qwerty287 I think there's not much value in further branch updated until this is resolved?

@qwerty287
Copy link
Contributor Author

Tbh I personally think owners should only exist because they're necessary, but they shouldn't have additional permissions like a veto. All maintainers should have the same possibilities. If the maintainers can't come to a conclusion, it's fine that an owner's vote counts more or something like this. You still need owners of course because we have servers and online accounts etc. And thus I also think the name "owners" is not the best, "admins" would be better.

@6543
Copy link
Member

6543 commented Nov 25, 2024

another important piece of note: I personally wana add Mercurial support for a long time but it's just low priority as it requires to:

  • create an official Mercurial clone plugin
  • add support for sourcehut ...

and this is not as important as #3411 or #2254 (I already have some non ready code moving towards it) ...

... so if we merge this pull ... we would soon have to revert it and remigrate to it, so I don't see why we should do so

@xoxys
Copy link
Member

xoxys commented Nov 25, 2024

And again, why not implement it as official forge extension?

@qwerty287
Copy link
Contributor Author

This is not even necessary. The information that woodpecker stores here is used nowhere. You can use any plugin/container for cloning.

@6543
Copy link
Member

6543 commented Nov 25, 2024

This is not even necessary. The information that woodpecker stores here is used nowhere. You can use any plugin/container for cloning.

well

// add default clone step
if !c.local && len(conf.Clone.ContainerList) == 0 && !conf.SkipClone && len(c.defaultClonePlugin) != 0 {
cloneSettings := map[string]any{"depth": "0"}
if c.metadata.Curr.Event == metadata.EventTag {
cloneSettings["tags"] = "true"
}
container := &yaml_types.Container{
Name: defaultCloneName,
Image: c.defaultClonePlugin,
Settings: cloneSettings,
Environment: make(map[string]any),
}
for k, v := range c.cloneEnv {
container.Environment[k] = v
}
step, err := c.createProcess(container, backend_types.StepTypeClone)
if err != nil {
return nil, err
}
stage := new(backend_types.Stage)
stage.Steps = append(stage.Steps, step)
config.Stages = append(config.Stages, stage)

should use it to decide based on the default plugins what to use ...

@6543
Copy link
Member

6543 commented Nov 25, 2024

... so yes this needs adoption in this case ...

@6543
Copy link
Member

6543 commented Nov 25, 2024

well you could argue with altering #4352 to not have a slice but store the DefaultClonePlugin that can be changed ... and is set on enabling an repo (this logic could adopt to check what vcs is used) you could remove it ...

... but still in this case e.g. plugins like ready-release-go will just fail with no clear error to point users to

checking against CI_REPO_SCM==git can display a nice error msg ... same as missuse of the wrong clone plugin

@6543
Copy link
Member

6543 commented Nov 25, 2024

and extensions are we... like DKMS modules to kernels ... they get the job done but fail over and over and just shift the burden to the sysadmins - I wana have first class support for different VCS ? what argument is against that?

@qwerty287
Copy link
Contributor Author

Just the maintenance effort. We can't have first-class support for everything. As @xoxys said already, we can't even fix the bugs in the current codebase already.

@6543
Copy link
Member

6543 commented Nov 25, 2024

If it's just that nobody beside me wana maintain that ... then just let me maintain it i guess ...

@xoxys
Copy link
Member

xoxys commented Nov 25, 2024

Can we just do a poll? Sorry, but everyone already wanted someone in Woodpecker that was rejected, and it seems this discussion is going nowhere.

@6543
Copy link
Member

6543 commented Nov 25, 2024

PS: this code part aint one that created issues till now btw...

@pat-s
Copy link
Contributor

pat-s commented Nov 25, 2024

Mercurial or any other VCS are super niche and maybe have a user base of <5%, maybe under 2%?

We never had active requests for other VCS AFAIK. Most maintainers have likely never used anything else than git. Hence I don't understand why we would want to keep/extend this part.

Besides, I don't understand why you (@6543) are defending this PR so tremendously? It feels there must be something we don't see/know, also because you haven't acted similarly in the past for other PRs which seemed potentially more important.

Last, only having one person being responsible for such an important cornerstone of a codebase feels risky and might incur more complicated discussions in the future, for both users and maintainers.

Overall, I don't have a strong opinion on this personally but I can't see good (enough) arguments so far why it should be kept.

@zc-devs

This comment was marked as off-topic.

@qwerty287
Copy link
Contributor Author

Extensions, LOL. Even config service has not been working since start of the year.

I don't get your point. Addon forges should work and for cloning you can use anything as I said already.

@pat-s
Copy link
Contributor

pat-s commented Dec 5, 2024

@qwerty287 I am not really sure what this PR is waiting on. I see you rebase frequently. So, we are stuck here somehow and nobody wants to act I guess?

@qwerty287
Copy link
Contributor Author

Yes well I kinda still wait for a decision. I just update my branch to not get too many conflicts, but that's no big task...
We should discuss this owner-veto thing or do a vote among maintainers about this change.

@xoxys
Copy link
Member

xoxys commented Dec 16, 2024

@woodpecker-ci/maintainers please vote on this post for this PR. 👍 merge 👎 close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking will break existing installations if no manual action happens refactor delete or replace old code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants