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

Fix multiple small (but sometimes serious) issues #1144

Merged
merged 4 commits into from
Jul 26, 2021

Conversation

oturpe
Copy link
Contributor

@oturpe oturpe commented Jul 25, 2021

Master branch has multiple issues small (but sometimes very serious).

Vagrant development environment setup failed during 'vagrant up'

Errors during from task Build the Anitya documentation:

user-guide.rst:210:Inline strong start-string without end-string.
user-guide.rst:3:Duplicate explicit target name: "sourceforge.net"

Like the errors say, the entry about Sourceforge (git) was corrupted
in two different ways:

  1. Strong string was started with **, but never terminated.
    The fix is obvious.
  2. The same document had two links using format '`text `_',
    which is not allowed in reStructured Text.
    Fixed by using '__' in the second link, making it "anonymous".

Fix corrupted javascript

Caused the page it was on malfunction in serious ways.

Display error correctly when Test Check fails

When Test Check failed in the Edit Project page,
resulting error was displayed as '[Object object]'.
This was a result of concatenating an object to string.
Fixed by using JSON.stringify().

@oturpe oturpe requested a review from a team as a code owner July 25, 2021 10:37
@softwarefactory-project-zuul
Copy link

Build succeeded.

@oturpe oturpe force-pushed the fix-doc-build-failure branch from a0d889b to 026691f Compare July 25, 2021 17:43
@softwarefactory-project-zuul
Copy link

Build succeeded.

@oturpe oturpe force-pushed the fix-doc-build-failure branch from 026691f to 4636f69 Compare July 25, 2021 19:07
@oturpe oturpe changed the title Vagrant development environment setup failed during 'vagrant up' Fix multiple small (but sometimes serious) issues Jul 25, 2021
@softwarefactory-project-zuul
Copy link

Build succeeded.

Copy link
Contributor

@Zlopez Zlopez left a comment

Choose a reason for hiding this comment

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

The changes are looking good, could you just add a news file to this PR?

Name it PR1144.bug and add it to news folder. The text could be something like Fix documentation and javascript issue`

@@ -207,8 +207,8 @@ The backends available are:
You need to provide **Sourceforge name** if the name on RSS feed is different then the
project name on Sourceforge.

* **Sourceforge (git) for projects hosted on
`sourceforge.net <https://sourceforge.net>`_
* **Sourceforge (git)** for projects hosted on
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this wasn't caught by the tests. It was recently added in #1134.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was too. I looked at it again, Zuul configuration is wrong,
job anitya-tox-docs is actually running Bandit.
Probably just a copy paste error and easy to fix?

Otherwise, Vagrant uses the Makefile in the docs folder
while Zuul runs some commands embedded inside tox.ini.
That could cause varying results, too.

- job:
    name: anitya-tox-docs
    ...
    parent: tox
    run: ci/test-tox.yaml
    vars:
      tox_env: bandit
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

This really looks like a copy paste error on my side. I will fix it. I will also unify how the docs are built in vagrant and in tox.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the makefile and the docs tox target and they are almost identical, the tox one is even running ./generate_db_schema. So I would just fix the zuul file.

@@ -377,7 +377,7 @@ <h1>{{ context }} project</h1>
$(btn).show(); $(btn + "-spinner").hide();
alert(
'Unable to retrieve the latest version from upstream!\n'
+ 'ERROR: ' + res.responseJSON );
+ 'ERROR: ' + JSON.stringify(res.responseJSON) );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not that familiar with javascript, so this will be probably my error. Thanks for fixing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are not the only one 😆
This is a very common mistake in error handlers.
The default string resulting from adding an object to string is the useless [Object object]
and it is even easier to accidentally write that kind of code
because e.g. console.log(object) actually prints something useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why people are using some JavaScript replacements on serious projects?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to get rid of Javascript entirely, but I'm not frontend developer, so this is not my cup of tea.

oturpe added 4 commits July 26, 2021 11:13
Vagrant development environment setup failed during 'vagrant up'
with the following errors during from task 'Build the Anitya documentation':

> user-guide.rst:210:Inline strong start-string without end-string.
> user-guide.rst:3:Duplicate explicit target name: \"sourceforge.net\"

Like the errors say, the entry about 'Sourceforge (git)' was corrupted
in two different ways:
1. Strong string was started with `**`, but never terminated.
   The fix is obvious.
2. The same document had two links using format '`text <url>`_',
   which is not allowed in reStructured Text.
   Fixed by using '__' in the second link, making it "anonymous".

Signed-off-by: Otto Urpelainen <[email protected]>
Caused the page it was on malfunction in serious ways.

Signed-off-by: Otto Urpelainen <[email protected]>
When Test Check failed in the Edit Project page,
resulting error was displayed as '[Object object]'.
This was a result of concatenating an object to string.
Fixed by using JSON.stringify().

Signed-off-by: Otto Urpelainen <[email protected]>
@oturpe oturpe force-pushed the fix-doc-build-failure branch from 4636f69 to ffad6dd Compare July 26, 2021 08:14
@softwarefactory-project-zuul
Copy link

Build succeeded.

@oturpe
Copy link
Contributor Author

oturpe commented Jul 26, 2021

The changes are looking good, could you just add a news file to this PR?

Name it PR1144.bug and add it to news folder. The text could be something like Fix documentation and javascript issue`

Uh, sorry. I was not entirely sure if there is required or encouraged for every pull request. Fixed.

@Zlopez
Copy link
Contributor

Zlopez commented Jul 26, 2021

The changes are looking good, could you just add a news file to this PR?
Name it PR1144.bug and add it to news folder. The text could be something like Fix documentation and javascript issue`

Uh, sorry. I was not entirely sure if there is required or encouraged for every pull request. Fixed.

I don't create them, if this is only about fixing tests or some small changes in dev environment, but it's good to have them if this is anything that affects users of release-monitoring.org

@Zlopez
Copy link
Contributor

Zlopez commented Jul 26, 2021

Let me first fix the documentation build so I can see that it really fails, then I will merge this PR.

@Zlopez
Copy link
Contributor

Zlopez commented Jul 26, 2021

The Zuul PR is #1147

@Zlopez
Copy link
Contributor

Zlopez commented Jul 26, 2021

The PR #1147 failed, so let's merge this one and see if it will pass.

@mergify mergify bot merged commit ff3e2db into fedora-infra:master Jul 26, 2021
@LenkaSeg
Copy link
Contributor

@oturpe Thanks for the fixes! I missed it totally.

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.

4 participants