Skip to content

Fixed issue #254 - auto "fireuping" sup-config, if there are no any configured sources #358

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.

Sign up for GitHub

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

Open
tradzik wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Fixed issue #254 - auto "fireuping" sup-config, if there are no any configured sources #358

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf9daf6
Fixed issue #254
tradzik Dec 28, 2014
e1af256
Fixed autostarting of sup-config
tradzik Dec 30, 2014
ca0ec64
Fixed index.lock as in disscussion
tradzik Dec 31, 2014
554404f
Added eMBee's suggestion
tradzik Jan 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions bin/sup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,18 @@ end
module_function :start_cursing, :stop_cursing

Index.init
Index.lock_interactively or exit

Copy link
Copy Markdown
Member

@gauteh gauteh Dec 30, 2014

Choose a reason for hiding this comment

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

Why are you doing this? Make sure you understand all the implications of the changes you are doing as well as testing it. Changing this line is not something you do without careful thought and a good overview of how sup works.

begin
Redwood::start
Redwood::SourceManager.load_sources
Copy link
Copy Markdown
Member

@gauteh gauteh Jan 18, 2015

Choose a reason for hiding this comment

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

Is this not done twice now (in case of configured)?


if Redwood::SourceManager.sources.empty?
debug "No sources found!"
exec("sup-config")
abort
end

Index.lock_interactively or exit
Copy link
Copy Markdown
Member

@gauteh gauteh Dec 30, 2014

Choose a reason for hiding this comment

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

I see you have moved it down here, this might be ok. But this is after Redwood::start, and requires extensive testing.

Copy link
Copy Markdown
Author

@tradzik tradzik Dec 30, 2014

Choose a reason for hiding this comment

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

I have tested this solution on two kinds of email sources and it worked. I don't see any problem there, everything should work as expected.

Copy link
Copy Markdown
Member

@gauteh gauteh Dec 30, 2014

Choose a reason for hiding this comment

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

You change the time of mail index locking, this might have other
consequences. Why did you change it in the first place? You also move it
into a begin/rescue block where it was not previously.

On Tue, Dec 30, 2014 at 1:05 PM, Tymon Radzik notifications@github.com
wrote:

In bin/sup
#358 (diff):

begin
Redwood::start

  • Redwood::SourceManager.load_sources
  • if Redwood::SourceManager.sources.empty?
  • debug "No sources found!"
  • exec("sup-config")
  • abort
  • end
  • Index.lock_interactively or exit

I have tested this solution on two kinds of email sources and it worked. I
don't see any problem there, everything should work as expected.

—
Reply to this email directly or view it on GitHub
https://github.com/sup-heliotrope/sup/pull/358/files#r22345721.

Copy link
Copy Markdown
Author

@tradzik tradzik Dec 30, 2014

Choose a reason for hiding this comment

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

It wasn't there, but on about 6 tests I it worked I expected. If see any issue there, please give me exact example, where is error.

Copy link
Copy Markdown
Contributor

@terotil terotil Dec 31, 2014

Choose a reason for hiding this comment

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

I second @gauteh's question: Why the locking needed to be moved? Apparently you had a good reason to do it. Otherwise you wouldn't have done it, right? So, please share the rationale with us and we'll understand too.

Of course no errors have been triaged that have this move-of-locking as root cause. The point is, however, that as a general rule, one should be conservative in making changes to avoid ripple effects. If there's no gain from the change, why take the potential risk of breaking things in subtle ways which we may only find out later on the hard way?

Copy link
Copy Markdown
Author

@tradzik tradzik Dec 31, 2014

Choose a reason for hiding this comment

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

@terotil As I saw, locking-index could cause problems, when running with sup-config. Espetically, when index is in bad version... That's why I moved this option after my if. Risk is minimal there, it is tested, I think it is unlikely possible to be anye errors there

Copy link
Copy Markdown
Contributor

@terotil terotil Dec 31, 2014

Choose a reason for hiding this comment

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

@ty221 I see. How about releasing the lock before exec("sup-config")?

I think starting the initialization procedure without locking index is bad idea in principle and I'd like to see that sequence not to be touched because of smoother exception handling.

Copy link
Copy Markdown
Author

@tradzik tradzik Dec 31, 2014

Choose a reason for hiding this comment

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

That might be good idea. I will try to implement it @terotil .

Index.load
Redwood::check_syncback_settings
Index.start_sync_worker unless $opts[:no_threads]
Expand Down Expand Up @@ -432,5 +440,4 @@ EOS
puts e.message, e.backtrace
end
end

end