-
Notifications
You must be signed in to change notification settings - Fork 287
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
Add support for repo archiving #1165
Conversation
7f2fc30
to
1952a25
Compare
It was only used for sequential iteration.
Archived repositories are loaded from `repos/archive`.
It could now be a problem because we store repositories under the `archive` directory. It could also produce an error before though, if someone set the same org/name combination for two repositories in two different files.
1952a25
to
d3672e9
Compare
@@ -98,6 +104,7 @@ impl<'a> Generator<'a> { | |||
}) | |||
.collect(), | |||
branch_protections, | |||
archived, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... Do we want to include archived repos in the repos endpoint or continue to only show current repos and have a dedicared archived_repos
endpoint?
I think I would prefer the dedicated endpoint approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we don't even have an endpoint for archived teams. So maybe we should not even expose the archived repos as an endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not including the archived status may cause issues for sync-team, since it won't know the difference between a normal repo, an archived repo, and a repo that is not yet tracked.
(IMHO, I think a flag is sufficient since this only really matters for sync-team, and it is also similar to the way GitHub's API works.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, of course, we need to expose that information, that's the whole point of this PR, silly me :) The flag will definitely make both team
and sync-team
much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just seems simpler to me to keep that information completely separate (in 2 endpoints) rather than exposing this IMO very fundamental difference as a simple boolean. However, I don't want to block on this so I'll mark the PR as ready to merge if others disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think visibility is as fundamental as archived state. An archived repo is effectively only there for historical purposes. It's not actively worked on or interacted with in any way. It is in essence only related to non-archived repos in that it used to be a non-archived repo; otherwise, there's essentially no interaction with an archived repo (e.g., no PRs, no new issues, no branches, no CI runs, etc.). Everything we want to control about a repo essentially does not apply to archived repos, because archived repos are not interacted with outside of rare cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That raises a good point. Should archived repos be interactable and have any properties set at all? E.g., what happens if you change a branch protection for an archived repo? Should that do something? (Will that unarchive the repo automatically?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was definitely working under the assumption that archived repos are in a read-only state (more or less by definition), and in order to change them, you would need to unarchive them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine if sync-team makes changes to archived repos. It won't unarchive them, and the API seems to be fine with making changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: I don't think it matters much. There might be small things, for example the description. Someone may want to change that, and I think it would be kinda fussy to have to unarchive, change, and then re-archive. But I don't expect that to happen much, so I think it probably doesn't matter.
Archived repos are in
repos/archive
. Currently there is no validation that the repo contains no team access, I can add it if you want.