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

Stop closing forks / PRs - instead look at refreshing fork #72

Open
justinharringa opened this issue Jun 6, 2019 · 6 comments
Open

Comments

@justinharringa
Copy link
Contributor

As mentioned in #32 we probably should not be closing out existing PRs / forks because if there are multiple images that a repository depends on, it should receive at least one PR for all updates or a PR for each update.

The impetus for closing them was to handle situations where files had moved or there were potentially other conflicts. (See comment)

Could simply create new PRs based off of a new branch and leave around old ones or try and resolve conflicts within (which seems like a more difficult task).

@justinharringa
Copy link
Contributor Author

justinharringa commented Jun 6, 2019

Note: we could clear out any PRs where DFIU no longer detects particular dependencies. That could potentially be its own command.

Example

We see a PR that is updating alpine but we no longer detect alpine as a dependency in the fresh version.

@justinharringa
Copy link
Contributor Author

justinharringa commented Jun 6, 2019

It could clear out any PRs where it's currently running for a particular dependency.

Example

We're updating alpine and see that we have another PR for alpine. We could choose to either update that PR or close it out in favor of a new one. I would think that it'd be better to update the current PR but that may look funny in the PR tab.

@npwolf
Copy link

npwolf commented Jun 6, 2019

I think @justinharringa and I are on the same page, but I am going to re-state the behavior I would want to be extra clear:
DFIU of imageX should close out any previous PRs updating imageX before creating the latest PR to update imageX. It should not touch PRs updating imageY.

@justinharringa
Copy link
Contributor Author

Since I've kind of called out 2 different cases, it seems like when implementing the fix, we could decide to spawn new issues if it's too big to fit this in one story. But I'm ok with spawning a new issue as well.

@justinharringa
Copy link
Contributor Author

justinharringa commented Jul 12, 2019

Perhaps a way to handle this would be to create a branch with the full image name (e.g. myregistry.something.com/namespace/image:tag@sha256:123456789abcdef... - but would need to be cleansed of '@', ':', and any other illegal character) and then PR with that branch. If that branch already exists, just ensure that the branch is up to date with master via rebase.

Duplicate PRs for the same image name with a different tag/digest could be considered something the repo owners could handle manually or could be resolved via another issue/feature here.

@justinharringa
Copy link
Contributor Author

justinharringa commented Jul 18, 2019

@afalko @npwolf perhaps Dependabot has already done the heavy lifting and there is no longer a need for dockerfile-image-update?

https://github.com/dependabot/dependabot-core/tree/master/docker

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

No branches or pull requests

2 participants