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

Updated developers list and enabled CD | plugin-simplify-qa-connector #3685

Merged
merged 6 commits into from
Feb 9, 2024

Conversation

Simplify3x
Copy link
Contributor

@Simplify3x Simplify3x commented Dec 10, 2023

Link to GitHub repository

https://github.com/jenkinsci/simplify-qa-connector-plugin

When modifying release permission

If you are modifying the release permission of your plugin or component, fill out the following checklist:

Release permission checklist (for submitters)

When enabling automated releases (cd: true)

Follow the documentation to ensure, your pull request is set up properly. Don't merge it yet.
In case of changes requested by the hosting team, an open PR facilitates future reviews, without derailing work across multiple PRs.

Link to the PR enabling CD in your plugin

jenkinsci/simplify-qa-connector-plugin#9

CD checklist (for submitters)

Reviewer checklist

There are IRC Bot commands for it.

@Simplify3x Simplify3x requested a review from a team as a code owner December 10, 2023 18:57
@Simplify3x Simplify3x changed the title plugin-simplify-qa-connector | Updated developers list and enabled CD Updated developers list and enabled CD | plugin-simplify-qa-connector Dec 10, 2023
@Simplify3x Simplify3x mentioned this pull request Dec 11, 2023
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-12-11 at 12 58 24

The link should be the link to your plugin's PR, not the RPU PR.

@Simplify3x
Copy link
Contributor Author

@NotMyFault can you provide some clarification or maybe a link, as to which two branches need to be merged in the Plugin's PR?

@NotMyFault
Copy link
Member

NotMyFault commented Dec 12, 2023

NotMyFault can you provide some clarification or maybe a link, as to which two branches need to be merged in the Plugin's PR?

Which part is unclear? You prepare the changes outlined in the documentation, raise a PR including all plugin modifications towards your plugin, and we review these changes made. If everything is right, this PR will be merged.

@Simplify3x
Copy link
Contributor Author

The changes outlined in the documentation are already a part of the latest commit in the master branch of the repository.
There is no requirement to raise a PR.
You are good to review the code changes.

@NotMyFault
Copy link
Member

NotMyFault commented Dec 13, 2023

The changes outlined in the documentation are already a part of the latest commit in the master branch of the repository.
There is no requirement to raise a PR.

That exactly is the requirement, allowing us to review them before integrating into the default branch. Because, as in your case, the changes made are incomplete and cannot be reviewed in a PR-like manner.

If you read over the documentation and see the examples PRs, for example the JUnit one. Please create a new PR towards your plugin with all these changes allowing us to review them, as recommended.

@Simplify3x
Copy link
Contributor Author

Hi @NotMyFault,
We have made the changes and created a new PR as per mentioned in the documentation.
Kindly review the changes and provide your valuable feedback.

@NotMyFault
Copy link
Member

Looks good so far, please rebase your PR with the latest changes on the default branch.

@Simplify3x
Copy link
Contributor Author

Rebase Done

@NotMyFault
Copy link
Member

Rebase Done

Screenshot 2023-12-18 at 21 19 03

This PR has not been rebased.

@Simplify3x
Copy link
Contributor Author

Simplify3x commented Dec 18, 2023

We could not find the update with rebase option so we merged it instead.
Trying to revert and re-create a new PR.

@Simplify3x
Copy link
Contributor Author

Can we get some update on this?

@alecharp
Copy link
Contributor

@Simplify3x please, on your local clone of this repository (repository-permissions-updater) the following commands:

git pull -a
git rebase upstream/master

if the remote towards https://github.com/jenkins-infra/repository-permissions-updater.git is not called upstream on your local clone, please adapt the command accordingly.

@Simplify3x
Copy link
Contributor Author

Simplify3x commented Dec 21, 2023

@Simplify3x please, on your local clone of this repository (repository-permissions-updater) the following commands:

git pull -a
git rebase upstream/master

if the remote towards https://github.com/jenkins-infra/repository-permissions-updater.git is not called upstream on your local clone, please adapt the command accordingly.

All the desired changes have been already pushed onto this fork (remote) towards https://github.com/jenkins-infra/repository-permissions-updater.git

Now, to rebase and merge these changes to the master branch of RPU, it is only possible by you or your team.
We do not have push access to the master branch of RPU

@alecharp
Copy link
Contributor

@Simplify3x when you open a pull request against a repository (jenkins-infra/repository-permissions-updater), you link a branch from your fork and the pull request. Anything that you push to the branch will be seen in the pull request. So, when you merged the commits from jenkins-infra:master into your master branch, and push that commit into your fork, it's showed here (e0a0408).

Now, to rebase and merge these changes to the master branch of RPU, it is only possible by you or your team.
We do not have push access to the master branch of RPU

You don't need write access on the jenkins-infra repository to do that. That's why we asked you do to the rebase.

Comment on lines 7 to 8
- "io/jenkins/plugins/simplify-qa-connector"
- "io/jenkins/plugins/OldUtil"
- "io/jenkins/plugins/SQA"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is something you want to include in your pull request.

Copy link
Contributor Author

@Simplify3x Simplify3x Jan 12, 2024

Choose a reason for hiding this comment

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

We want to include that.

Copy link
Member

Choose a reason for hiding this comment

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

We want to include that.

The response lacks a sufficient justification of what the path is supposed to represent in terms of enabling CD.

Copy link
Contributor Author

@Simplify3x Simplify3x Jan 24, 2024

Choose a reason for hiding this comment

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

What kind of justification do you require?
What is your acceptance criteria?

Copy link
Member

Choose a reason for hiding this comment

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

What kind of justification do you require?

The change proposed lacks a rationale reasoning why you want to modify your release paths, when the PR description and title claim to enable CD only.

Additionally, the repository isn't set up to support project inheritance (i.e., no reactor, no module aggregation, etc.). This leads to the fact that you can't release multiple artifacts under multiple artifact IDs, even if you want to do that.
It's worth mentioning that this is a complex procedure, you commonly don't answer with "We want to include that.", and therefore I agree with alecharp that you should remove this from the PR.

@NotMyFault NotMyFault enabled auto-merge (squash) February 9, 2024 12:25
@NotMyFault NotMyFault merged commit 8253c2e into jenkins-infra:master Feb 9, 2024
3 checks passed
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.

3 participants