InnerSourceCommons / archive.innersourcecommons.org

The old version of the ISC site

Creative Commons Attribution Share Alike 4.0 International

40 stars 29 forks source link

Removed absolute path usage from _head.html #102

Closed dellagustin closed 4 years ago

dellagustin commented 4 years ago

This PR partly addresses #115

Remarks

Once again I forgot to branch from master, so this is including #98 and #99 , but I expect them to be integrated before this one.
In light of the results mentioned in https://github.com/InnerSourceCommons/innersourcecommons.org/issues/100#issuecomment-590098579, I am not sure it is worth to put the effort of continuing this work, but I need to try that first.

rrrutledge commented 4 years ago

Awesome work!

Is it possible to see this change deployed to GitHub pages before merge so as to very that things still work?

lenucksi commented 4 years ago

Awesome work!

Is it possible to see this change deployed to GitHub pages before merge so as to very that things still work?

The changes built on CI which means that Jekyll didn't crash and burn. If the site looks the same is something that would need to be validated by yourself locally, with a stage deployment or some Selenium excess.

Stage deployment would be a second, non-public deployment e.g. using Netlify and a second branch, in other words:

You would first pull request against our staging branch that would be deployed do the non-public deployment
and then upon acceptance of the result merge the staging branch into the master with a second pull request that triggers the deployment to the public website.

dellagustin-sap commented 4 years ago

@rrrutledge that could be done manually, but I would be the equivalent of merging it and letting the build pipeline deploy it.

Reviewing it locally and having exactly the same experience you have when you deploy to the productive website is virtually impossible.

We could try to increase test coverage, we would have to be very smart about it, not to commit to stuff we are not sure we want to keep, otherwise, when you refactor code, you also have to refactor test code.

lenucksi commented 4 years ago

Reviewing it locally and having exactly the same experience you have when you deploy to the productive website is virtually impossible.

I would argue that in the current state of things a manual and local preview will be sufficient in nearly all cases. It if breaks, we can go back very fast. It's not like we're operating the Google front page including search with that website in my opinion. ;) I wouldn't complain about having an automated staging deployment though. :D

dellagustin commented 4 years ago

@lenucksi I am of the very same opinion.

rrrutledge commented 4 years ago

Sounds good. Just check the altered links after deployment?

lenucksi commented 4 years ago

When should we merge this? Once all the checkboxes are ticked?

dellagustin commented 4 years ago

@rrrutledge

Sounds good. Just check the altered links after deployment? Sure

@lenucksi

When should we merge this? Once all the checkboxes are ticked?

Good question, what happens if you merge now and I push another commit to the branch that originated this PR? Would it be reopened? Is that an acceptable GitHub workflow? I never did it like this, but it "smells bad".

Maybe I should create an issue with the title "Refactor: reduce usage of absolute paths", transfer the checkboxes there and we go merging bite size PRs (as this one) with each individual correction.

Any opinions?

rrrutledge commented 4 years ago

Having an issue sounds great!

lenucksi commented 4 years ago

@lenucksi

When should we merge this? Once all the checkboxes are ticked?

As soon as we can, i.e. as soon as the merge-conflict that exists currently is handled.

Good question, what happens if you merge now and I push another commit to the branch that originated this PR?

A cute baby kitten dies. ;)

Would it be reopened? Is that an acceptable GitHub workflow? I never did it like this, but it "smells bad".

No, I don't think so. No. Yes it does in my book too. ;)

Maybe I should create an issue with the title "Refactor: reduce usage of absolute paths", transfer the checkboxes there and we go merging bite size PRs (as this one) with each individual correction.

Any opinions?

Sounds fine to me. Please close this PR if you're going to split it the things up. If you reference this PR in the new PRs that would be very useful :D I'm all for small PRs. Most of the times.