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

Amend publish script #15

Closed
wants to merge 3 commits into from
Closed

Amend publish script #15

wants to merge 3 commits into from

Conversation

gemmaleigh
Copy link
Contributor

git commit -am was not adding the toolkit files, try using git add -A, then git commit to achieve this.

git commit -am was not adding the toolkit files, try using git add -A, then git commit to achieve this.
@tombye
Copy link
Contributor

tombye commented Feb 10, 2017

I was looking into the problem this is intended to solve and @gemmaleigh pointed me to this pull request.

I found a few things that I think might help this along.

What's causing the tests to fail

When npm version $VERSION_LATEST is run $VERSION_LATEST is the same as the version in the package.json so it can't bump it and fails. This is causing the travis build to fail.

Not all changes made to the govuk_frontend_toolkit code are being carried across

For example, part of alphagov/govuk_frontend_toolkit#293 (in version 5.0.0) was the removal of the images/external-links folder. Looking at the images folder you can see this change was not carried across to this repo.

This is because of how the updates from govuk_frontend_toolkit are added by publish.sh.

The process goes like this:

  1. govuk_frontend_toolkit is checked out into a new folder
  2. after unnecessary files are removed, the rest is copied into the root of this repo & the checkout is removed
  3. any changes detected in the working tree are added to a commit bumping the package version

The problem seems to lie with step 2. Without removing what's currently in the root of this repo before, copying across whatever files comprise the latest govuk_frontend_toolkit means deletions don't show up.

Suggested changes

I've some ideas about possible solutions to all the problems listed so far but I have a feeling there might be better ones so be interested in some discussion.

Here's a few ideas for what to do next:

Deletions not showing up

We add a rm -rf * step to publish.sh to remove all non-dotfiles prior to copying across the govuk_frontend_toolkit files. After copying we can git checkout -- publish.sh package.json to get those files back. If this is combined with @gemmaleigh's changes, this should mean all types of change will be staged when the version bump is committed.

The tests failing

We bump the version by a patch number.

We're not introducing any new features or bug fixes to the govuk_frontend_toolkit code but the NPM module currently doesn't contain an accurate version of 5.0.3 so marking the fix is important.

@gemmaleigh
Copy link
Contributor Author

@tombye this is super helpful, thank you! I'll look at this properly next week.

publish.sh Outdated
VERSION_LATEST=`cat VERSION.txt`
VERSION_REGISTRY=`npm view govuk_frontend_toolkit version`

if [ "$VERSION_LATEST" != "$VERSION_REGISTRY" ]; then
git commit -am "Temporary commit: new toolkit files"
git add -A
Copy link
Contributor

Choose a reason for hiding this comment

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

For the npm publish this makes sense, but I'm not sure what the git behaviour will be here after rm -rfing earlier.

Will files removed upstream be removed in the commit that gets pushed at the end, or remain after the reset? Honestly not sure what the behaviour is without running it locally myself - it'd be good to confirm that before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading a bit more it looks like -a should resolve deleted into the commit, based on git add -A behaviour, i'm still not 100% sure it'll commit 'new' files from upstream, although it may through git reset --soft.

I'll try running it locally after lunch to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running it locally, I get the following output:

 delete mode 100644 CONTRIBUTING.md
 delete mode 100644 README.md
 rename images/{accordian-arrow-2x.png => accordion-arrow-2x.png} (100%)
 rename images/{accordian-arrow.png => accordion-arrow.png} (100%)
 create mode 100644 images/crests/dit_crest_13px.png
 create mode 100644 images/crests/dit_crest_13px_x2.png
 create mode 100644 images/crests/dit_crest_18px.png
 create mode 100644 images/crests/dit_crest_18px_x2.png
 create mode 100644 images/crests/dit_crest_27px.png
 create mode 100644 images/crests/dit_crest_27px_x2.png
 delete mode 100644 images/external-links/external-link-24x24.png
 delete mode 100644 images/external-links/external-link-black-12x12.png
 delete mode 100644 images/external-links/external-link-black-24x24.png
 delete mode 100644 images/external-links/external-link.png
 create mode 100644 images/icon-arrow-left.png
 create mode 100644 images/separator-2x.png
 create mode 100644 images/separator.png
 create mode 100644 javascripts/govuk/analytics/analytics.js
 create mode 100644 javascripts/govuk/analytics/download-link-tracker.js
 create mode 100644 javascripts/govuk/analytics/error-tracking.js
 create mode 100644 javascripts/govuk/analytics/external-link-tracker.js
 create mode 100644 javascripts/govuk/analytics/google-analytics-universal-tracker.js
 create mode 100644 javascripts/govuk/analytics/mailto-link-tracker.js
 create mode 100644 javascripts/govuk/analytics/print-intent.js
 create mode 100644 javascripts/govuk/modules.js
 create mode 100644 javascripts/govuk/modules/auto-track-event.js
 create mode 100644 javascripts/govuk/shim-links-with-button-role.js
 create mode 100644 javascripts/govuk/show-hide-content.js
 create mode 100644 stylesheets/_helpers.scss
 create mode 100644 stylesheets/colours/_organisation.scss
 create mode 100644 stylesheets/colours/_palette.scss
 create mode 100644 stylesheets/design-patterns/_breadcrumbs.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v5.0.0 removed the external link icons and renamed the accordion images.
https://travis-ci.org/alphagov/govuk_frontend_toolkit/builds/170405690

It looks like those files haven't been updated, so this was a problem before the publish script stopped publishing this repo to npm.

Copy link
Contributor

@tombye tombye Feb 17, 2017

Choose a reason for hiding this comment

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

Yeah, the images/external-links folder was still present when we recently checked out the latest NPM package. This was what alerted me to the problem.

I think what causes confusion is that we're copying over files, not carrying over commits. This means if a folder, like images/external-links is removed from the upstream, when the files/folders are checked out and copied across the only action that results is it doesn't get copied across because it's not there. This leaves the images/external-links folder in this repo untouched.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it was a problem before then it shouldn't hold up this PR, but we should look at it after tho.

Do this before adding untracked files, to ensure files are deleted - if they have been deleted from the govuk_frontend_toolkit repo.
Add comments for additional steps.
@gemmaleigh
Copy link
Contributor Author

Closing in favour of @dsingleton's PR.

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.

None yet

3 participants