-
Notifications
You must be signed in to change notification settings - Fork 20
Deprecate automatic tagging #136
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
Conversation
There was a problem hiding this 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
|
@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 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. |
|
The current travis process builds and minifies the JS files as part of the Again, read the blog post about why we did it this way
This is your pull request, so I would like to hear your ideas |
|
@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 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
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 ( 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? |
|
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 |
|
I am fine with these changes since it ensures the front end assets are built and minified.
Does this need to be added to the |
|
https://travis-ci.org/cfpb/ccdb5-ui/builds/431586924#L717 I am guessing that's the change and the dependency is not needed |
|
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:
Regarding
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.
ed5dc18 to
60b56a9
Compare
|
Yeah, since this is not a npm package, I guess we can stop tracking the version |
Deprecate automatic tagging
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 installis 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