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

Hide eol distros #204

Merged
merged 17 commits into from May 22, 2018
Merged

Hide eol distros #204

merged 17 commits into from May 22, 2018

Conversation

mikaelarguedas
Copy link
Contributor

This PR is a replacement for previously reverted #169

  • uses loaded distributions to generate Package and Stack headers
  • uses the already existing list of active distributions (distro_names_buildfarm) to create the ros version selector

Pros:

Cons:

  • no way for user to access wiki pages for not displayed distros

Alternative:
Create a checkbox "display EOL distros" next to the version selector that adds back all distros to the version selector and allow to access content for all of them

wjwwood
wjwwood previously approved these changes Mar 27, 2017
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

The code changes lgtm.

But, this:

  • no way for user to access wiki pages for not displayed distros

Seems like a pretty big con to me. I don't know how others feel about it, but I'm hesitant to move forward without the ability to access the wiki blocks for older distributions.

@tfoote
Copy link
Member

tfoote commented Mar 27, 2017

In the meeting we talked about refactoring this to hide the EOL'd distro selectors, but not completely disable them.

@mikaelarguedas
Copy link
Contributor Author

Delaying this until less radical approach is implemented

@dhood
Copy link
Contributor

dhood commented Jul 21, 2017

hey @mikaelarguedas and/or @wjwwood @tfoote, how were you testing this PR?

I have a local instance of the wiki, and was going to create a test page that uses that macro, but first I have to create a user and I haven't been able to do that successfully yet (haven't tried disabling the whitelist yet)

Did you test on a local instance, or just on the live one?

@mikaelarguedas
Copy link
Contributor Author

I was testing live for short time periods

@dhood
Copy link
Contributor

dhood commented Jul 21, 2017

ok, thanks!

@mikaelarguedas mikaelarguedas dismissed wjwwood’s stale review May 4, 2018 02:37

changed the approach and @dhood is implementing the alternative

@dhood
Copy link
Contributor

dhood commented May 4, 2018

  • Now there's a checkbox that is unchecked by default but if checked, the version selector for eol distros appears.

  • content-embedding macros for all distros are supported (e.g. diamondback_and_newer can be used even though it doesn't have a version selector).

  • If the distro selected in the url is an EOL distro, the eol selector is displayed on page load.

  • If a package has only been released into eol distros, the checkbox isn't there.

example stack header with only some distros released (eol distros are hidden by default)
package_header


example stack header with only EOL distros released
package_header_eol_only


example Version() (eol distros are hidden by default)
version_macro

@@ -14,6 +14,8 @@
distro_names = ['boxturtle', 'cturtle', 'diamondback', 'electric', 'fuerte', 'groovy', 'hydro', 'indigo', 'jade', 'kinetic', 'lunar', 'melodic', 'unstable']
distro_names_indexed = ['diamondback', 'electric', 'fuerte', 'groovy', 'hydro', 'indigo', 'jade', 'kinetic', 'lunar', 'melodic', 'unstable'] #boxturtle and cturtle not indexed
distro_names_buildfarm = ['indigo', 'jade', 'kinetic', 'lunar', 'melodic']
distro_names_never_on_buildfarm = ['boxturtle', 'cturtle', 'diamondback', 'unstable']
Copy link
Contributor

Choose a reason for hiding this comment

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

I just took a guess at why these distros are being excluded from the macros, open to suggestions for a more appropriate variable name if there is one

@dhood
Copy link
Contributor

dhood commented May 4, 2018

With jade removed from the non-EOL list, Version header looks like:

by default:
version_jade_eol_collapsed

expanded:
version_jade_eol

@dhood
Copy link
Contributor

dhood commented May 4, 2018

I actually think that cturtle and diamondback have been removed from the version selector by accident somewhere along the line (@mikaelarguedas pointed out all files are modifying the global distro_names, so that would do it).

Our ubuntu installation page has code to load the cturtle/diamondback installation pages: http://wiki.ros.org/Installation/Ubuntu (which it can't do currently because the version selector starts at electric).

I definitely think that we should restore support for macros referencing older distros e.g. http://wiki.ros.org/image_transport has a section using diamondback_and_newer that is not currently being displayed. This PR does that. But, do we also want to add cturtle and diamondback back to the (EOL) version selector?


btw this is the contents of the wiki page I previously posted screenshots for, which is what's making the text show up:

<<Version()>>

{{{{#!wiki version kinetic
$ROS_DISTRO
}}}}

{{{{#!wiki version diamondback_and_newer
as of diamondback
}}}}
{{{{#!wiki version indigo_and_newer
as of indigo
}}}}

And the following for PackageHeader (I'm testing on a dummy wiki which is why it gives index errors):

<<PackageHeader(rviz)>>

{{{{#!wiki version kinetic
$ROS_DISTRO
}}}}

{{{{#!wiki version diamondback_and_newer
`my_api()` <<Version(ROS diamondback)>>
}}}}

@dhood dhood added the in review label May 4, 2018
@dhood
Copy link
Contributor

dhood commented May 16, 2018

This PR now highlights that EOL distros are EOL.

Version() macro:

version_macro_highlight_eol

PackageHeader:

package_header_eol_only2

@dhood
Copy link
Contributor

dhood commented May 21, 2018

@tfoote @mikaelarguedas @wjwwood could any of you (re)review this if you get a chance

@dhood dhood merged commit ed8bbf3 into master May 22, 2018
@dhood dhood deleted the hide_eol_distros branch May 22, 2018 19:37
@dhood dhood self-assigned this May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants