Bug #6967
closed[API] [Workbench] application.default.yml should not call git, at least in production
Added by Brett Smith about 9 years ago. Updated about 9 years ago.
Description
Both API server and Workbench call git a couple of times in application.default.yml
:
source_version: "<%= `git log -n 1 --format=%h`.strip %>" local_modified: "<%= '-modified' if `git status -s` != '' %>"
These calls don't work on deployments from our packages. Worse, source_version
causes a crash when starting the server if git is not installed or not in the user's $PATH. At the very least, the production section should get all these without calling git.
Updated by Brett Smith about 9 years ago
- Project changed from 35 to Arvados
- Story points set to 0.5
Updated by Brett Smith about 9 years ago
Plan:
- Update API server and Workbench packages to include a file with the version.
- The production section of application.default.yml should try to set source_version to the contents of that file.
- The production section of application.default.yml should set local_modified to false.
Updated by Nico César about 9 years ago
In production we currently have:
hieradata/uuid_cluster/common.yaml that has the version array including arvados-src
Brett what's your opinion about adding it to the application.yml.erb (in API and Workbench)?
Updated by Nico César about 9 years ago
this bug causes random fatal: Not a git repository (or any of the parent directories): .git
when executing the console, see:
Updated by Brett Smith about 9 years ago
- Target version changed from Arvados Future Sprints to 2015-09-30 sprint
Updated by Brett Smith about 9 years ago
- Target version changed from 2015-09-30 sprint to Arvados Future Sprints
Updated by Brett Smith about 9 years ago
- Target version changed from Arvados Future Sprints to 2015-09-30 sprint
Branch 6967-application-yml-without-git-wip is up for review.
Brett Smith wrote:
Plan:
- Update API server and Workbench packages to include a file with the version.
This has already been done for a while.
- The production section of application.default.yml should try to set source_version to the contents of that file.
I couldn't quite do this, because all the ERB in application.default.yml is run without regard to the current Rails environment. Trying to always read this file in the production section would break development instances.
Instead, the code tries to get version information from Git first, since it's most comprehensive; then it falls back to trying the provided file if that fails for any reason. Users will not see Git error output either way.
- The production section of application.default.yml should set local_modified to false.
This is already done. This is set in the common section, and only the development section overrides it.
Updated by Radhika Chippada about 9 years ago
Comments for commit 44042f1b80c65e09aa94c738c0ac385b365b0669
- I think you can do a break in the "git_pipe.each_line do" loop after setting "status_output = true"
- It appears that the code block that computes source_version and local_modified at the top of api and workbench default yml file is the same. Would it be possible to create a new yml file such as "get_source_version.yml" with just this code block in one dir (let's say api/config) and link to it in workbench/config dir and include that file contents or results into the application.default.yml file?
Updated by Brett Smith about 9 years ago
Radhika Chippada wrote:
Comments for commit 44042f1b80c65e09aa94c738c0ac385b365b0669
- I think you can do a break in the "git_pipe.each_line do" loop after setting "status_output = true"
It's important to continue reading all of the output from the pipe. Otherwise, the git process may get killed by SIGPIPE and report errors when it would otherwise run fine.
- It appears that the code block that computes source_version and local_modified at the top of api and workbench default yml file is the same. Would it be possible to create a new yml file such as "get_source_version.yml" with just this code block in one dir (let's say api/config) and link to it in workbench/config dir and include that file contents or results into the application.default.yml file?
You're right that the code is identical between files. I would like to refactor it, but unfortunately changes like you suggest don't play well with our packaging infrastructure. Symlinks in the Workbench source are included verbatim in the Workbench packages, so symlinks to code outside the Workbench tree (i.e., in the API server) will become dangling links when the package is installed. That would make it impossible for the Workbench application.default.yml to include the code when installed from a package.
Updated by Radhika Chippada about 9 years ago
Brett: sounds good. LGTM (though I did not test by restarting my servers). Thanks.
Updated by Brett Smith about 9 years ago
- Status changed from New to In Progress
Updated by Brett Smith about 9 years ago
Brett Smith wrote:
- The production section of application.default.yml should try to set source_version to the contents of that file.
I couldn't quite do this, because all the ERB in application.default.yml is run without regard to the current Rails environment. Trying to always read this file in the production section would break development instances.
But I can fake it by introspecting Rails.env directly. I've pushed a change that does this, and adds some comments to help clarify some of the issues Radhika brought up. Now at ffd589a.
Updated by Nico César about 9 years ago
I reviewed 744c0c3..f311437
LGTM
I verbally commented to Brett about the crons that excecute in random directories and we tried to come up with potential problems... but we think is safe enough...
Updated by Brett Smith about 9 years ago
Nico Cesar wrote:
I verbally commented to Brett about the crons that excecute in random directories and we tried to come up with potential problems... but we think is safe enough...
Specifically, I think our use of Bundler prevents the script from being launched outside the Rails root. But I decided better safe than sorry, and adjusted the code to make sure we always launch git from the Rails root. Pushed to master with that change in 285c0092. Thanks.
Updated by Brett Smith about 9 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:5a55113805af906145449be18ec86b1a95a5017b.
Updated by Tom Clegg about 9 years ago
Reopening to rearrange this code so it doesn't cause trouble when non-Erb YAML parsers try to read application*.yml
This should really be a gem, meanwhile at least I've moved it to lib/app_version.rb
6967-yaml-format @ 965c5b3
(incl. bonus fix: "rake config:check" was printing the blob_signing_key value in the clear instead of *******
)
Updated by Tom Clegg about 9 years ago
- Status changed from Resolved to In Progress
- Target version changed from 2015-09-30 sprint to 2015-10-14 sprint
Updated by Tom Clegg about 9 years ago
- Assigned To changed from Brett Smith to Tom Clegg
Updated by Peter Amstutz about 9 years ago
Reviewing 6967-yaml-format:
- You should use
git status --porcelain
instead ofgit status -s
, from the man page:
--porcelain Give the output in an easy-to-parse format for scripts. This is similar to the short output, but will remain stable across Git versions and regardless of user configuration.
- Suggest adding
--untracked-files=no
togit status
so that the presence of untracked files don't cause it to report "modified" - Turns out I had
git-commit.version
left over from package building, I was initially very puzzled why it was reporting the wrong version. Perhaps it should try git before looking forgit-commit.version
- apps/workbench/lib/app_version.rb has the following copy and paste error at the top:
# If you change this file, you'll probably also want to make the same # changes in apps/workbench/lib/app_version.rb.
(not having two copies of the essentially the same file would be even better, but whatever.)
Updated by Tom Clegg about 9 years ago
Peter Amstutz wrote:
- You should use
git status --porcelain
instead ofgit status -s
, from the man page:
Sure, "regardless of user config" sounds better. Fixed.
- Suggest adding
--untracked-files=no
togit status
so that the presence of untracked files don't cause it to report "modified"
Idk, seems to me if you have untracked files (that aren't .gitignored) then your source tree is modified. I'm thinking "added a new source file but haven't committed it yet". I'm guessing you are thinking of a different kind of example.
Either way, I'd like to stick to the bugfix here rather than turn it into an opportunity to request/add features. "Can't run python tests any more" is the reason this got reopened.
- Turns out I had
git-commit.version
left over from package building, I was initially very puzzled why it was reporting the wrong version. Perhaps it should try git before looking forgit-commit.version
Hm, the subject here is "don't call git, at least in production" so I don't think that would fly.
Meanwhile, at least tests now fail if you have git-commit.version in your source tree, which could make that less puzzling than it was before.
- apps/workbench/lib/app_version.rb has the following copy and paste error at the top:
Fixed
(not having two copies of the essentially the same file would be even better, but whatever.)
Agreed
Updated by Tom Clegg about 9 years ago
- Status changed from In Progress to Resolved
- % Done changed from 50 to 100
Applied in changeset arvados|commit:1d73ebb1fb2f2f50144de4f74b3aa77616a5d4f2.