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

Revert support for ASSET_\d+_URL #1988

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

foursixnine
Copy link
Member

@foursixnine foursixnine commented Feb 6, 2019

For the time being seems that the dynamic schedule of tests with
caching seems a bit complicated. Mainly due to some tests using ASSET_\d
as part of an URL and others using it as a file to be expected to be
downloaded. The cache service should only treat assets as something to
download.

The feature introduced in #1855 is nice, but won't work with caching for
the time being. See https://progress.opensuse.org/issues/43511.

Note: This feature broke autoyast tests :)

One idea that I have is to introduce a separate ASSET_LOCAL_ for the cases when we expect the cache service to actually download the thing and link it to the worker pool.

PS: With #1943 and subsequent related pr's, we wouldn't need to specify the ASSET_X_URL, which makes all of this even easier.

@codecov

This comment has been minimized.

@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #1988 into master will increase coverage by 17.45%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1988       +/-   ##
===========================================
+ Coverage   71.62%   89.07%   +17.45%     
===========================================
  Files         132      154       +22     
  Lines        9617    10436      +819     
===========================================
+ Hits         6888     9296     +2408     
+ Misses       2729     1140     -1589
Impacted Files Coverage Δ
lib/OpenQA/Worker/Engines/isotovideo.pm 94.5% <ø> (-0.61%) ⬇️
lib/OpenQA/Parser/Format/Base.pm 100% <0%> (ø)
lib/OpenQA/WebAPI/Controller/API/V1/Mm.pm 58.33% <0%> (ø)
lib/OpenQA/WebAPI/Controller/API/V1/Bug.pm 95.91% <0%> (ø)
lib/OpenQA/Parser/Format/XUnit.pm 98.36% <0%> (ø)
lib/OpenQA/Worker/Cache/Service.pm 73.52% <0%> (ø)
lib/OpenQA/Worker/Cache/Task/Asset.pm 100% <0%> (ø)
lib/OpenQA/Script.pm 90.47% <0%> (ø)
lib/OpenQA/WebAPI/Plugin/Fedmsg.pm 96.84% <0%> (ø)
... and 73 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 616cc17...b4b0c6b. Read the comment docs.

@foursixnine
Copy link
Member Author

@coolo any opinions against this?

@coolo
Copy link
Contributor

coolo commented Feb 12, 2019

Opinions against commented code? Yes. Don't do that.

For the time being seems that the dynamic schedule of tests with
caching seems a bit complicated. Mainly due to some tests using ASSET_\d
as part of an URL and others using it as a file to be expected to be
downloaded. The cache service should only treat assets as something to
download.

The feature introduced in os-autoinst#1855 is nice, but won't work with caching for
the time being. See https://progress.opensuse.org/issues/43511
@foursixnine
Copy link
Member Author

@coolo :) Fixed. Now, question here:

One idea that I have is to introduce a separate ASSET_LOCAL_ for the cases when we expect the cache service to actually download the thing and link it to the worker pool.

What do you think about this? Or you would rather pick up another path? (Like not exploring it at all?)

@coolo
Copy link
Contributor

coolo commented Feb 12, 2019

I suggest not to try with the current worker.

@foursixnine foursixnine merged commit c393803 into os-autoinst:master Feb 12, 2019
@foursixnine
Copy link
Member Author

Self merge :) Should fix the broken fix.

@foursixnine foursixnine deleted the need-less-cache branch February 12, 2019 10:14
coolo pushed a commit that referenced this pull request Feb 12, 2019
commit c393803
Merge: b4bb54e b4b0c6b
Author:     Santiago Zarate <santiago@zarate.co>
AuthorDate: Tue Feb 12 11:13:56 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Feb 12 11:13:56 2019 +0100

    Merge pull request #1988 from foursixnine/need-less-cache

    Revert support for ASSET_\d+_URL
coolo pushed a commit that referenced this pull request Feb 14, 2019
commit c393803
Merge: b4bb54e b4b0c6b
Author:     Santiago Zarate <santiago@zarate.co>
AuthorDate: Tue Feb 12 11:13:56 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Feb 12 11:13:56 2019 +0100

    Merge pull request #1988 from foursixnine/need-less-cache

    Revert support for ASSET_\d+_URL
coolo pushed a commit that referenced this pull request Feb 16, 2019
commit c393803
Merge: b4bb54e b4b0c6b
Author:     Santiago Zarate <santiago@zarate.co>
AuthorDate: Tue Feb 12 11:13:56 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Feb 12 11:13:56 2019 +0100

    Merge pull request #1988 from foursixnine/need-less-cache

    Revert support for ASSET_\d+_URL
@okurz
Copy link
Member

okurz commented Feb 17, 2019

@foursixnine Deleting code that does not make the tests fail is a good approach anyway ;)

coolo pushed a commit that referenced this pull request Feb 18, 2019
commit c393803
Merge: b4bb54e b4b0c6b
Author:     Santiago Zarate <santiago@zarate.co>
AuthorDate: Tue Feb 12 11:13:56 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Feb 12 11:13:56 2019 +0100

    Merge pull request #1988 from foursixnine/need-less-cache

    Revert support for ASSET_\d+_URL
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.

None yet

3 participants