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

Final updates for the thesis #405

Merged
merged 5 commits into from May 9, 2018

Conversation

Projects
None yet
2 participants
@mikicz
Copy link
Member

mikicz commented May 8, 2018

  1. Updated to Arca v0.2, once I publish the package, including setting timeouts
  2. Add some missing tests
  3. Fix some validation issues
  4. Removed one extra condition

@mikicz mikicz requested a review from hroncok May 8, 2018

@@ -9,7 +9,7 @@
{% if course is defined %}
<header class="lesson-header">
<a href="{{ url_for('index') }}">Nauč se Python </a>
{% if course is defined and course.vars.get('coach-present') %}
{% if course.vars.get('coach-present') %}

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

How is this relevant? Isn't it breaking anything?

This comment has been minimized.

@mikicz

mikicz May 8, 2018

Author Member

Well, it is not breaking anything, but the condition is redundant since the same one is three lines above that. But it was introduced by me in the main PR or in some follow-up...

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

I see. OK. I'd rather see it in different commit but that's juts me having OCD.

This comment has been minimized.

@mikicz

mikicz May 8, 2018

Author Member

Done

@@ -599,7 +599,8 @@ class CourseLink(CourseMixin, Model):
branch: str = DataProperty(link, default="master")

info = ForkProperty(repo, branch, entry_point="naucse.utils.forks:course_info",
args=lambda instance: [instance.slug])
args=lambda instance: [instance.slug],
timeout=1)

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

OK, can we rather keep it default and override it in travis config if it takes to long?

This comment has been minimized.

@mikicz

mikicz May 8, 2018

Author Member

This argument is passed along to the Task instance, meaning it cannot be overridden by environment variables

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

I see. Let's keep it this way then. However, why 1 sec? Can it fail on us on Travis if it is a bit slower sometime?

This comment has been minimized.

@mikicz

mikicz May 8, 2018

Author Member

The timeout is only applied to task execution itself, meaning excluding startup of the container, I find it hard to believe that once the container is started this particular function would fail to complete in a second - it only loads a file, deserializes it from YAML, takes a couple of values and returns them in a JSON.

That said I am not dead set on having this timeout, I'm fine with removing it and keeping the 5-sec default from Arca...

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

I'm not dead set against having it 1 second for now. I'm just saying: Be prepared to make it less strict once it starts being problematic. I've seen Travis CI builds where it got stuck for 2 seconds without any obvious reason. Cloud is somebody else's computer, you never know what is running there during our deployment.

This comment has been minimized.

@mikicz

mikicz May 8, 2018

Author Member

Ok, we'll see what happens once I release v0.2 of Arca and the build in this branch won't fail because it can't install Arca...

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

Would you like to use a tarball requirement for now before you do a release?

This comment has been minimized.

@hroncok

hroncok May 8, 2018

Member

Timeout in tests :)

This comment has been minimized.

@mikicz

mikicz May 8, 2018

Author Member

Was just about to comment on it. I will revert it to the default...

@mikicz mikicz force-pushed the mikicz:arca-v02 branch 3 times, most recently from 58aedcc to 51efdfe May 8, 2018

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented May 8, 2018

The whole build was successful after the 1-second timeout was removed

@mikicz mikicz force-pushed the mikicz:arca-v02 branch from 51efdfe to 70f2051 May 9, 2018

@mikicz mikicz changed the title WIP: Final updates for the thesis Final updates for the thesis May 9, 2018

@mikicz mikicz force-pushed the mikicz:arca-v02 branch from 70f2051 to a3128b7 May 9, 2018

@hroncok hroncok merged commit 4dbb1f7 into pyvec:master May 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment