Bug #6967

[API] [Workbench] application.default.yml should not call git, at least in production

Added by Brett Smith almost 5 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
09/23/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

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.


Subtasks

Task #7480: Review 6967-yaml-formatResolvedPeter Amstutz

Task #7375: Review 6967-application-yml-without-git-wipResolvedBrett Smith

Associated revisions

Revision 5a551138
Added by Brett Smith over 4 years ago

Merge branch '6967-application-yml-without-git-wip'

Closes #6967, #7375.

Revision 1d73ebb1
Added by Tom Clegg over 4 years ago

Merge branch '6967-yaml-format' closes #6967

Revision b8afbe37 (diff)
Added by Tom Clegg over 4 years ago

6967: Update test to match improved code.

refs #6967

History

#1 Updated by Brett Smith almost 5 years ago

  • Project changed from Arvados Private to Arvados
  • Story points set to 0.5

#2 Updated by Brett Smith almost 5 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.

#3 Updated by Nico César almost 5 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)?

#4 Updated by Nico César almost 5 years ago

this bug causes random fatal: Not a git repository (or any of the parent directories): .git when executing the console, see:

https://arvados.org/issues/7047#note-3

#5 Updated by Brett Smith over 4 years ago

  • Target version changed from Arvados Future Sprints to 2015-09-30 sprint

#6 Updated by Brett Smith over 4 years ago

  • Target version changed from 2015-09-30 sprint to Arvados Future Sprints

#7 Updated by Brett Smith over 4 years ago

  • Assigned To set to Brett Smith

#8 Updated by Brett Smith over 4 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.

#9 Updated by Radhika Chippada over 4 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?

#10 Updated by Brett Smith over 4 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.

#11 Updated by Radhika Chippada over 4 years ago

Brett: sounds good. LGTM (though I did not test by restarting my servers). Thanks.

#12 Updated by Brett Smith over 4 years ago

  • Status changed from New to In Progress

#13 Updated by Brett Smith over 4 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.

#14 Updated by Nico César over 4 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...

#15 Updated by Brett Smith over 4 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.

#16 Updated by Brett Smith over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:5a55113805af906145449be18ec86b1a95a5017b.

#17 Updated by Tom Clegg over 4 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 *******)

#18 Updated by Tom Clegg over 4 years ago

  • Status changed from Resolved to In Progress
  • Target version changed from 2015-09-30 sprint to 2015-10-14 sprint

#19 Updated by Tom Clegg over 4 years ago

  • Assigned To changed from Brett Smith to Tom Clegg

#20 Updated by Peter Amstutz over 4 years ago

Reviewing 6967-yaml-format:

  • You should use git status --porcelain instead of git 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 to git 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 for git-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.)

#21 Updated by Tom Clegg over 4 years ago

Peter Amstutz wrote:

  • You should use git status --porcelain instead of git status -s, from the man page:

Sure, "regardless of user config" sounds better. Fixed.

  • Suggest adding --untracked-files=no to git 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 for git-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

54f9295

#22 Updated by Tom Clegg over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

Applied in changeset arvados|commit:1d73ebb1fb2f2f50144de4f74b3aa77616a5d4f2.

Also available in: Atom PDF