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

Adding WikiCompress #506

Merged
merged 6 commits into from
Nov 11, 2019
Merged

Adding WikiCompress #506

merged 6 commits into from
Nov 11, 2019

Conversation

ElSiipo
Copy link
Contributor

@ElSiipo ElSiipo commented Oct 27, 2019

Retrieving compressImagesMessages queue, adding entry if WikiCompress is enabled.

Not sure if this is enough to prevent recursion?
if (repoConfiguration.CompressWiki && !parameters.CloneUrl.Contains(".wiki.gi"))

@dabutvin
Copy link
Contributor

hey nice start here!

Check the compile error in RepoCheckTests to handle the new parameter you added

@dabutvin
Copy link
Contributor

good call on preventing the infinite loop here 👍

@dabutvin
Copy link
Contributor

I just ran through a test of this and what we got so far is working great! Good job 😄

There's not too much left to wrap this feature up. Based on the discussion in #461, we need to skip the branch change for the wiki clone since there is no pull request UI or branch management for wikis.

There are 2 places where we deal with the branch in the CompressImages routine
Look for:
Commands.Checkout(repo, KnownGitHubs.BranchName);
We need to instead stay on the default branch we get to right after clone.

Also, we should return false from the routine instead of true so we do not queue a message for opening a PR to the next component.

And lastly I think we could use a log statement right where we are adding to compressImagesMessages to track that we got a wiki request queued, see logger.LogInformation for how to do this.

Let me know how it goes 👍

@ElSiipo
Copy link
Contributor Author

ElSiipo commented Oct 30, 2019

I decided to still return true if compression went fine, but added If-statement to see if it was for a Wiki repo to still do some logging in CompressImagesFunction..

Probably would be nice to do some splitting, since it starts to become quite some if-statements. 😸

@dabutvin
Copy link
Contributor

Great idea. Makes more sense to return true for didCompress 👍

For sure, if it ends up getting any crazier than this we should fully split.

We landed some changes to the way default branch is decided yesterday. I saw you resolved the conflicts! I'll make sure everything is playing nicely together and then merge it all in

Awesome work!

@dabutvin dabutvin merged commit a162a42 into master Nov 11, 2019
@dabutvin dabutvin deleted the add-support-for-compression-of-wiki branch November 11, 2019 00:22
@dabutvin
Copy link
Contributor

Got this all tested and integrated now!
Check out the commit I added on the end to handle some of the branching weirdness

6e0e360

@ElSiipo
Copy link
Contributor Author

ElSiipo commented Nov 11, 2019

Nice job!

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.

2 participants