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

beta waitlist #3

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

beta waitlist #3

wants to merge 18 commits into from

Conversation

mcull
Copy link

@mcull mcull commented Feb 19, 2018

Simple first implementation of in situ deployment waitlist for deploying to a stage

screen shot 2018-02-19 at 10 24 58 am

Tasks

  • 👍 from team

References

Risks

  • Level: Low

@mcull mcull requested a review from jdoconnor February 19, 2018 18:25
@@ -0,0 +1,13 @@
require 'clockwork'

Choose a reason for hiding this comment

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

have we considered using periodical, which is already built in?

Copy link
Author

Choose a reason for hiding this comment

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

no, but only because not familiar with it.

embarassed--google search coming up dry. can you add link?

Choose a reason for hiding this comment

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

require './config/boot'
require './config/environment'

include Clockwork

Choose a reason for hiding this comment

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

please ensure we're logging to a place that gets rotated (such as log/production.log)

end
end

def add

Choose a reason for hiding this comment

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

do we want to consider RESTful routes? I think add and remove make sense, but i'm just asking if we have strong opinions either way

end

def add
Rails.logger.warn("current_waitlist: #{current_waitlist.list}")

Choose a reason for hiding this comment

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

this is good for our debugging, but in a PR to zendesk, i would advise making this debug


#Samson::Hooks.view :stage_form, '<view path>'

Samson::Hooks.view :stage_show, 'samson_deploy_waitlist/deployers'

Choose a reason for hiding this comment

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

lets remove the stuff we're not actively using. They'll be potentially out of date with newer versions

next if s.lock.try(:user).try(:email) == current_head[:email]
next unless TIME_TO_LOCK_QUEUE.minutes.ago > waitlist.head_updated_at.to_datetime

# Current head of waitlist has been head for 5 minutes without locking the stage...

Choose a reason for hiding this comment

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

comment doesn't match constant time

TIME_TO_LOCK_QUEUE = 10

def self.check_your_head
Stage.all.each do |s|

Choose a reason for hiding this comment

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

please use stage as your iterator unless there's a better name for style


current_head = waitlist.list[0]

next if s.lock.try(:user).try(:email) == current_head[:email]

Choose a reason for hiding this comment

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

this is already bound to ruby > 2.4. You can use a safe operator
next if s.lock&.user&.email == current_head[:email]

@farvour
Copy link

farvour commented Jan 8, 2020

Do we care about this? It might need a rebase due to the pull from upstream zendesk into master.

@slpsys
Copy link

slpsys commented Jun 18, 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.

6 participants