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

2.7.1 unexpected return #443

Merged
merged 6 commits into from
Dec 5, 2018
Merged

2.7.1 unexpected return #443

merged 6 commits into from
Dec 5, 2018

Conversation

nllong
Copy link
Member

@nllong nllong commented Dec 3, 2018

When launching the rake commands, there was an 'unexpected return' that appeared in the command line. It was caused by calling openstudio openstudio_version in the environment files. This was setting the os_version, but was never used.

@nllong nllong requested a review from anyaelena December 3, 2018 22:32
@@ -6,4 +6,5 @@ echo "Waiting for Mongo to start"
echo "Waiting for Redis to start"
/usr/local/bin/wait-for-it --strict -t 0 queue:6379

cd /opt/openstudio/server && bundle exec rake environment resque:workers
# Only start a single worker when calling this script (do not use resque:workers).
Copy link
Member

Choose a reason for hiding this comment

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

if this is appropriate, we should rename the script (which is start-workers.sh). but is it used for for worker node setup as well as web-background?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, start-workers.sh only starts a single worker instance and registers it with redis. This was probably causing an issue because in docker-compose.yml (for local use) had COUNT=1 set then called resque:workers. Let me know if you want me to rename before merging.

Copy link
Member

Choose a reason for hiding this comment

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

preference for renaming but not required.

@coveralls
Copy link

coveralls commented Dec 3, 2018

Coverage Status

Coverage increased (+0.02%) to 66.448% when pulling 7d4e528 on 2.7.1-unexpected-return into ab15f93 on develop.

@@ -76,4 +76,7 @@

# Use a different logger for distributed setups.
# config.logger = ActiveSupport::TaggedLogging.new(SyslogLogger.new)

# TODO: Remove assumption that we are developing on a machine that has which installed! Make cross platform...
config.os_gemfile_path = File.expand_path(File.join(File.dirname(`which openstudio`), '../Ruby'))
Copy link
Member

Choose a reason for hiding this comment

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

i think this will fail on windows. also, it will trigger the --bundle options to be passed with openstudio cli call, which should only happen if the bundle has been installed locally and we do not want to use what comes packaged with openstudio

Copy link
Member Author

Choose a reason for hiding this comment

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

This will fail on windows, but I'm assuming that no one is developing on windows (or running the development.rb environment. That was the essence of my TODO comment.

Copy link
Member

Choose a reason for hiding this comment

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

I guess i'm more concerned with introduction of requirement that bundle install be run locally for the openstudio install. what is this getting us?

Copy link
Member

Choose a reason for hiding this comment

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

i have approved this pr, but for the record i think that at a minimum a very explicit comment about the bundle install requirement will help us in the future. or commit with this commented out and the explanation to uncomment if you want to run with a locally installed openstudio bundle.

@@ -6,4 +6,5 @@ echo "Waiting for Mongo to start"
echo "Waiting for Redis to start"
/usr/local/bin/wait-for-it --strict -t 0 queue:6379

cd /opt/openstudio/server && bundle exec rake environment resque:workers
# Only start a single worker when calling this script (do not use resque:workers).
Copy link
Member

Choose a reason for hiding this comment

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

preference for renaming but not required.

.travis.yml Outdated
@@ -13,7 +13,7 @@ gemfile:

# Set the default scripts that are used for the majority of the tests in the matrix
before_install:
- ./ci/travis/setup.sh
- travis_wait ./ci/travis/setup.sh
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@nllong nllong force-pushed the 2.7.1-unexpected-return branch 4 times, most recently from 336ec66 to 61fc630 Compare December 5, 2018 02:25
@nllong nllong merged commit 931a9b5 into develop Dec 5, 2018
@nllong nllong deleted the 2.7.1-unexpected-return branch December 5, 2018 04:47
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.

3 participants