Opened 3 years ago

Closed 15 months ago

#3545 closed change (incomplete)

Don't hardcode retrieval of subscriptionlist content in get_subscriptions()

Reported by: greiner Assignee:
Priority: P4 Milestone:
Module: Websites Keywords:
Cc: kvas Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by kvas)

Background

Testing subscriptionlist changes on adblockplus.org/subscriptions requires changes to web.adblockplus.org. More specifically, the subscriptionlist repository content is fetched from the remote source. So to be able to refer to a local source it requires changes to globals/get_subscriptions.py which includes hardcoded URLs to the source.

What to change

  1. Take the URL of the repository from settings.ini of the website. The key should be called subscriptions_repo. The default value of the URL should be https://hg.adblockplus.org/subscriptionlist (the repo that is used now).
  2. In case the subscriptions repo is remote, use the strategy that is used now (fetch settings file individually and get the rest as a .tgz archive).
  3. In case the subscriptions repo is local, read the files from the currently checked out working copy. Any non-committed changes would also be included.
  4. If the environment variable CMS_SUBSCRIPTIONS_REPO is set, it should override the value from settings.ini (or the default). This would allow pointing the script to a local subscriptions repository without changing any files in the website repository.

Change History (20)

comment:1 Changed 3 years ago by juliandoucette

  • Component changed from Websites to Sitescripts

comment:2 Changed 3 years ago by greiner

  • Component changed from Sitescripts to Websites
  • Description modified (diff)

This code is part of the web.adblockplus.org repository (see globals/get_subscriptions.py). Apparently, it was moved to a different directory though so I updated the path that's mentioned in the issue description to reflect that.

comment:3 Changed 3 years ago by juliandoucette

  • Priority changed from Unknown to P4
  • Ready set

comment:4 Changed 3 years ago by juliandoucette

This looks doable.

@kvas can you please confirm the priority of this issue (I think it's P4) and feel free to tackle it. Otherwise, I fear it will be here a while.

Last edited 3 years ago by juliandoucette (previous) (diff)

comment:5 Changed 3 years ago by juliandoucette

  • Cc kvas added

comment:6 Changed 3 years ago by juliandoucette

(Sorry @kzar, I meant @kvas.)

comment:7 Changed 3 years ago by kzar

  • Cc kzar removed

Sure, no problem.

comment:8 follow-up: Changed 3 years ago by kvas

Two questions:

  • I'm not sure why you want to use sitescripts.ini here. It seems that this configuration variable has nothing to do with sitescripts (or am I wrong?). Would not using settings.ini of the website make more sense?
  • Do you think that the default behavior (if no repository is specified in the configuration) should stay the way it is now?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by greiner

Replying to kvas:

  • I'm not sure why you want to use sitescripts.ini here. It seems that this configuration variable has nothing to do with sitescripts (or am I wrong?). Would not using settings.ini of the website make more sense?

At the time when the ticket was created the get_subscriptions() function was still part of the sitescripts repo which is why I suggested "sitescripts.ini" (possibly also to avoid duplication). From what I see it is now located in the web.adblockplus.org repo, however, so feel free to modify the ticket description accordingly.

  • Do you think that the default behavior (if no repository is specified in the configuration) should stay the way it is now?

It'd be helpful to get some warning or error to clarify what needs to be changed to get it to work but yeah, generally I don't expect the default behavior to change.

comment:10 in reply to: ↑ 9 Changed 3 years ago by kvas

  • Ready unset

It seems that the best way forward for now would be to change the logic of this script to clone the subscriptions repository and then work with the local clone instead of streaming the .tgz archive from the server and extracting the files on the fly. Then for testing we could just point the macro to a local copy using an environment variable or a key in the website settings.ini. I will check with the original author of the macro if this doesn't break any useful existing behavior and then the ticket would be ready to work.

comment:11 Changed 3 years ago by juliandoucette

  • Priority changed from P4 to Unknown

comment:12 Changed 3 years ago by juliandoucette

  • Priority changed from Unknown to P5

comment:13 Changed 3 years ago by kvas

Having thought about it again, I would say that it seems better to leave the default behavior as is (streaming .tgz file from server and unpacking it on the fly). It produces less network traffic and we don't need to worry about where to put the clone of the repo. The only thing to change here is that the url of the archive should be taken from settings.ini of the website.

Then, as we agreed, if the url is a local directory, we just read the subscriptions from there instead. And we can also add an environment variable for overriding the subscriptions url -- this would make debugging even easier.

If this seems fine, I can update the description of the issue and we can set it to Ready and give it a proper priority.

comment:14 Changed 3 years ago by juliandoucette

  • Priority changed from P5 to Unknown

comment:15 Changed 3 years ago by juliandoucette

If this seems fine, I can update the description of the issue and we can set it to Ready and give it a proper priority.

Seems fine to me.

comment:16 Changed 3 years ago by kvas

  • Description modified (diff)

comment:17 Changed 2 years ago by juliandoucette

  • Priority changed from Unknown to P4

comment:18 Changed 15 months ago by juliandoucette

  • Component changed from Websites to Sitescripts

I'm going to give this to you instead of migrating it to gitlab kvas. If you disagree please create an issue on gitlab or ping me on IRC.

comment:19 Changed 15 months ago by juliandoucette

  • Component changed from Sitescripts to Websites

Migrating this instead as discussed on IRC.

comment:20 Changed 15 months ago by juliandoucette

  • Resolution set to incomplete
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.