#113 CLI: don't attempt Pagure sync if API token is not set (#112)
Merged 4 years ago by frantisekz. Opened 4 years ago by adamwill.
fedora-qa/ adamwill/blockerbugs no-default-pagure  into  develop

file modified
+4
@@ -184,6 +184,10 @@ 

  

  

  def sync_discussions(args):

+     if app.config["PAGURE_REPO_TOKEN"] in ("YOUR SECRET API TOKEN FROM PROJECT SETTINGS", ""):

+         # skip this if the API token is set to the placeholder value

+         # or empty string as we are not going to be able to create anything

+         return

      active_milestones = Milestone.query.filter_by(active=True).all()

  

      for milestone in active_milestones:

It's not going to work, and trying will just waste a bunch of
time and network resources.

Signed-off-by: Adam Williamson awilliam@redhat.com

Pull-Request has been merged by frantisekz

4 years ago

Guess how quickly this will break? Immediately after someone modifies the sentence in the example config file.

Why don't you compare this to config.Config.PAGURE_REPO_TOKEN instead of a hardcoded string?

@adamwill @frantisekz Can you submit an improved version of this PR please?

Guess how quickly this will break? Immediately after someone modifies the sentence in the example config file.
Why don't you compare this to config.Config.PAGURE_REPO_TOKEN instead of a hardcoded string?
@adamwill @frantisekz Can you submit an improved version of this PR please?

This is a non issue, why would anybody ever modify that string ? Can you look into git history of any of our projects and take a look how often do strings like this change? I'd bet on "never".

Or take a look how we check if SECRET_KEY has been changed in production? Eg. resultsdb:

if app.config['PRODUCTION']:
    if app.secret_key == 'replace-me-with-something-random':
        print("Secret KEY is: " + app.secret_key)
        raise Warning("You need to change the app.secret_key value for production")

Feel free to submit a PR @kparal , I am not going to spend time on this....

You can either try to build reliable software that doesn't break when you change a comment, or you cobble stuff up and then you or other people waste time on unexpected sudden breakages from seemingly trivial unrelated changes. I think it's really embarrassing to be defending the latter approach, especially when the required change involves changing a couple words, but our views clearly differ.

Yes, I regularly go through config files in my projects and try to make them as readable and user friendly as possible, which sometimes includes changing exactly values like this one. The criticism was not about frequency, though, but the obscurity of what an innocent change might do. If the simple change of YOUR SECRET API TOKEN FROM PROJECT SETTINGS to YOUR SECRET API TOKEN FROM PAGURE PROJECT SETTINGS in setting.py.example (example!) breaks functionality, then the code is poorly written. And it doesn't really matter that we have the same poor code in resultsdb (we should fix it there as well).

If nobody else does, I'll fix it.

Just to step in - While I absolutely agree with @kparal about the technical aspect, I fell a part of @frantisekz's hasty answer was caused by the seemingly "You are stupid, hur dur" tone of the original comment, the #comment-114292 is IMO clearly unnecessarily hostile.

And while I understand that emotions are on the edge in these times, we should do our best to both phrase and read the texts with care.

@frantisekz / @adamwill please change the if-clause to if app.config["PAGURE_REPO_TOKEN"] in (config.Config.PAGURE_REPO_TOKEN, ""): (with the necessary boilerplate, ofc

@kparal please make sure to write the comments with care. I know I'm basically a pot calling the kettle black, but being unnecessarily sharp-tongued in a "we can't hear your tone, bro" situations might be uncalled for.

I rewrote my answer several times and considered my reply very gentle :-) But, thanks for stepping in.

note, that clause is kinda unnecessary ass-covering anyway. I originally wrote the clause to just skip the sync if the value is "YOUR SECRET API TOKEN FROM PROJECT SETTINGS", and it didn't work, and I was angry. Then I realized the script that bootstraps a development environment writes PAGURE_REPO_TOKEN = '' into conf/settings.py anyway, so pretty much everyone is going to have that. I just left the other string in the check out of our old friend An Abundance Of Caution.

I can change it, but then, isn't it about equally likely someone will refactor the Config class as someone will change the string value? Still, I suppose it'd fail more visibly in that case...

I can change it, but then, isn't it about equally likely someone will refactor the Config class as someone will change the string value?

IMO the core idea here is, that instead of having "magic string" in three places we only have it in two (which are also "connected" on an basic idea level, at least).

sure, I'm just pointing out that there's a chance of it breaking either way. Someone could refactor the config class but keep the "magic string value" the same, in which case the old way (this PR) would keep working but the new one (#117) would fail.

Yes, it can break in a different way. But having 2 strings is better than 3. The proper solution is one of these two:
a) PAGURE_REPO_TOKEN should default to an empty string and we should use real comments for instructions. Then it's clear that when it is not empty, it should be used.
b) There should be an explicit boolean toggle for enabling/disabling discussion syncing in the config file, e.g. SYNC_DISCUSSIONS = True

b) was my initial suggestion here, but this covers what I need. I can see a toggle might still be useful if you have the token set but for some reason want to suppress syncing, I guess. I can't think of a situation where you'd want to override the default to True, though - why would you want to attempt sync with an empty or obviously invalid token?

I'm not sure if I understand. The idea behind b) is that the choice is explicit, hard to get mistaken about. If you try to enable syncing but provide an invalid token, that's your problem then, it was your deliberate choice. Of course it's still helpful if we go the extra mile, detect invalid tokens and print a warning to the terminal/logs, to help users figure out mistakes.

To be clear, if the current solution is satisfactory for your, this discussion is mainly academic ("how would I design this well?"). We don't need to implement that. Solution b) is simply a good pattern, so I mention it. From The Zen of Python: "Explicit is better than implicit." :wink: The fact that syncing is disabled when no proper token value is provided is an implicit behavior, it can be logical, but it's not obvious without documentation.

Metadata