Skip to content

Conversation

@chosak
Copy link
Member

@chosak chosak commented Sep 21, 2018

This change removes the automatic tagging behavior that currently runs on Travis after every push/merge to master. This makes the behavior of this repository more consistent with other cfgov-refresh satellite apps: on tags, Travis will upload the built wheel file to the tag's GitHub Release.

It also adds a command to .travis.yml that reverts any local changes made to package-lock.json when npm install is run. This is because package-lock.json might be modified due to environment differences between Travis and development machines. See this npm issue for some background for why this change is needed:

npm/npm#17722

See https://travis-ci.org/chosak/ccdb5-ui/builds/431516239 for a build run on my fork after a tag was manually made. The release wheel is cleanly named. The error on upload to GitHub is expected.

@JeffreyMFarley FYI. The pu.sh script is extremely clever and I think could be very useful on cfgov-refresh. These changes were made to bring this repo more in line with the other satellite apps.

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

Copy link
Contributor

@JeffreyMFarley JeffreyMFarley left a comment

Choose a reason for hiding this comment

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

cc: @sephcoster

Please leave the push script in there, but you can keep ignoring package-lock.json

The push script only runs on merges to master. It is intended to "lock in" a release from dev

More info: https://dev.to/jeffreymfarley/deploy-atomically-with-travis--npm-68b

@chosak
Copy link
Member Author

chosak commented Sep 21, 2018

@JeffreyMFarley all other cfgov-refresh satellites are distributed/included as wheel files. Are there other consumers of distributables that you are considering?

I do think this is a clever approach but it's important to have consistency across cf.gov satellites.

Additionally, this current implementation breaks the expected cf.gov wheel generation process. This section of the .travis.yml file currently never works, because when Travis runs on a tag, for example this one it skips the run because of the [skip ci]. See Travis request log here. So as-is this repo doesn't generate wheels as we'd like.

If you have some other suggestion that supports continuous tagging AND wheels, I'd be open to it. But we do need to have wheels on releases for consistent inclusion of this repo in cfgov-refresh.

See also cfpb/development#183 for some related discussion of the standard cfgov-refresh satellite release process.

@JeffreyMFarley
Copy link
Contributor

The current travis process builds and minifies the JS files as part of the npm version https://github.com/cfpb/ccdb5-ui/blob/master/package.json#L104.

Again, read the blog post about why we did it this way

If you have some other suggestion that supports continuous tagging AND wheels, I'd be open to it.

This is your pull request, so I would like to hear your ideas

@chosak
Copy link
Member Author

chosak commented Sep 21, 2018

@JeffreyMFarley now I see what I was missing! Thanks for that second comment. I didn't fully understand the frontend build process because I was again erroneously assuming it worked similarly to the other satellites.

Now I see that this repo includes the built frontend assets. In the other satellites, we only check in the source files, and then run the build at installpackage time.

See changes in 82efc5f -- this leverages our cfgov-django-setup package so that the frontend build is run at install time. How would you feel about this approach?

(Having the frontend build as part of the installpackage step is one reason we prefer installing from wheels instead of from GitHub for the satellites, so the build doesn't have to happen each time.)

Where are the changes that will generate the wheels

This logic already exists here in Travis: the wheel is generated here and then this section is supposed to upload the wheel to the GitHub Release on every new tag/release (tags: true). This last section does not work (and I guess has never worked) when someone manually makes a new tag/release because the commit before the tag has [skip ci] and so when Travis runs it just ignores it.

I certainly understand the desire to avoid the need for manual steps and would welcome a broader adoption of automation in all of our releases!

I spent some time considering how we might do wheels on top of the continuous tagging happening here, but I don't have a solution as yet. Whether maintaining continuous tagging is more important than satellite consistency depends on whether continuous tagging is uniquely needed for this repo. Are there other consumers of this repo's deliverables besides cfgov-refresh who need continuous tagging?

@chosak
Copy link
Member Author

chosak commented Sep 21, 2018

Edit to add: after writing this I see that there is one missing piece, which is the version number in package.json. I fear that we don't do a good job of maintaining this consistently in the satellite apps. We seem to even exclude that field in some (example), so perhaps it doesn't really matter as these packages aren't really npm installable in any real way.

@JeffreyMFarley
Copy link
Contributor

I am fine with these changes since it ensures the front end assets are built and minified.

this leverages our cfgov-django-setup package so that the frontend build is run at install time

Does this need to be added to the requirements.txt or setup.py?

@JeffreyMFarley
Copy link
Contributor

https://travis-ci.org/cfpb/ccdb5-ui/builds/431586924#L717

I am guessing that's the change and the dependency is not needed

@chosak
Copy link
Member Author

chosak commented Sep 21, 2018

Thanks @JeffreyMFarley. How would you feel about removing the version from package.json, as I've done in 60b56a9? According to the npm docs it is optional if not publishing to npm:

If you plan to publish your package, the most important things in your package.json are the name and version fields as they will be required. The name and version together form an identifier that is assumed to be completely unique. Changes to the package should come along with changes to the version. If you don't plan to publish your package, the name and version fields are optional.

Regarding

Does this need to be added to the requirements.txt or setup.py?

as of #134 this repo uses setup.py instead of requirements.txt and cfgov-django-setup is already included (it's published under the cfgov-setup name on PyPI).

This change removes the automatic tagging behavior that currently runs
on Travis after every push/merge to master. This makes the behavior of
this repository more consistent with other cfgov-refresh satellite apps:
on tags, Travis will upload the built wheel file to the tag's GitHub
Release.

It also adds a command to .travis.yml that reverts any local changes
made to package-lock.json when 'npm install' is run. This is because
package-lock.json might be modified due to environment differences
between Travis and development machines. See this npm issue for some
background for why this change is needed:

npm/npm#17722
Remove built frontend assets from the repo.
This package isn't going to be published to npm, so the version field is
optional. From https://docs.npmjs.com/files/package.json:

> If you plan to publish your package, the most important things in your
package.json are the name and version fields as they will be required.
The name and version together form an identifier that is assumed to be
completely unique. Changes to the package should come along with changes
to the version. If you don't plan to publish your package, the name and
version fields are optional.
@JeffreyMFarley
Copy link
Contributor

Yeah, since this is not a npm package, I guess we can stop tracking the version

@JeffreyMFarley JeffreyMFarley merged commit ade55a4 into cfpb:master Sep 21, 2018
@chosak chosak deleted the standardize-travis branch September 21, 2018 17:55
Scotchester pushed a commit that referenced this pull request Jul 20, 2020
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