Skip to content
This repository has been archived by the owner on Apr 13, 2021. It is now read-only.

NetKAN-bot Rewrite #19

Merged
merged 113 commits into from
Aug 13, 2015
Merged

NetKAN-bot Rewrite #19

merged 113 commits into from
Aug 13, 2015

Conversation

techman83
Copy link
Member

This is a full re-architect of the NetKAN indexing bot. It has feature parity (maybe slightly more) with the original single script.

closes #1
closes #3
closes #4
closes #5
closes #8
fixes #10

@techman83
Copy link
Member Author

Messed up the rebase/pull. But it's merge friendly again.

@techman83
Copy link
Member Author

Oh goodness, that commit history is really up the creek. Looks fine locally though O.o

# TODO: I think Git::Wrapper has a way to do this natively
# TODO: If not, wrap it in a try/catch
if ($self->shallow) {
system("git", "clone", "--depth", "1", $self->remote, $self->local."/".$self->working);
Copy link
Member

Choose a reason for hiding this comment

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

Git seems to give me a warning, when I'm running test cases, which may be related to this line.

warning: --depth is ignored in local clones; use file:// instead.

But that's not enough to be a blocker

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we use remotes and the test isn't specifically checking the depth of the pull, just that we can perform it. I will add a comment to the test.

@pjf
Copy link
Member

pjf commented Aug 11, 2015

@techman83 : I've added a couple more line comments, but they're more things which affect our tests rather than anything else.

It looked last my past self was pretty happy with this, and if the bot has issues we can always roll it back, and this is way better than our past architecture, so you should:

  1. Fix the line comments above if you want to. (Fixing the netkan.exe download URL is probably a must)
  2. Merge this and deploy.

It's silly for things like this to be delayed because I've been busy, and changes to our indexing and build systems are relatively low-risk compared to changes to our clients.

Thank you so much for your patience!

@techman83
Copy link
Member Author

Thanks for the review :-)

techman83 added a commit that referenced this pull request Aug 13, 2015
@techman83 techman83 merged commit d05d859 into KSP-CKAN:master Aug 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants