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

Data files from docs.cask.co #2

Merged
merged 15 commits into from
Nov 18, 2016
Merged

Data files from docs.cask.co #2

merged 15 commits into from
Nov 18, 2016

Conversation

awholegunch
Copy link
Contributor

@awholegunch awholegunch commented Nov 10, 2016

#New file is docs.cask.co/www/resources/scripts/redirect-page.js
Modified file is docs.cask.co/www/404.html which has been changed to use it

The redirect page and the modified 404 are to address https://issues.cask.co/browse/INFRA-31
The Error Document for the bucket is (and should be) set to this modified 404.html page.

Results at http://builds.cask.co/browse/DOC-SITE0-19/artifact

New file is docs.cask.co/www/resources/scripts/redirect-page.js
Modified file is docs.cask.co/www/404.html which has been changed to use it
Copy link
Contributor

@wolf31o2 wolf31o2 left a comment

Choose a reason for hiding this comment

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

A few minor changes, and a request to reduce duplication. Do we need multiple copies of jquery.js or the cask_404_75pc.png file? Can they not just go under /resources and all the docs that are on the site use them?

j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
'//www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
})(window,document,'script','dataLayer','GTM-KWLFGH');</script>
<!-- Google Tag Manager end -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this block stored in another file we can include, instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought this block is included in just about all of our files, I don't know if it can be separate. Looking at Google's docs, they never suggest including it that way. It might have something to do with latency and the asynchronous nature of that code block; it might need to be directly in the page rather than in a separate file. This was included following their directions.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. For some reason I was thinking that it was included in our docs, not in-line, but I think I am just mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you're not mistaken: it is included in our docs. But there it is the template file that Sphinx then uses to build the documentation. So each HTML page it as it is here. It is included in ~/Source/cdap/cdap-docs/_common/_themes/cdap/layout.html.

</script>
<script type="text/javascript" src="cdap/current/en/_static/jquery.js"></script>
<script type="text/javascript" src="cdap/current/en/_static/underscore.js"></script>
<script type="text/javascript" src="cdap/current/en/_static/doctools.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should any of this be at the root level? Maybe under resources/scripts? Seems like yes, to me.

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've made copies of these in the resources directory, and linked to them in the upper-level files as appropriate. There are still copies in the current directories, as per the comment below.

*/

(function() {
var products = new Array( "cdap", "coopr", "tigon" );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we read this list from a file that we generate? This could be generated by the TYPES list that gets run in the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The browser wouldn't be able to read the file, but we could generate this file (redirect-page.js) dynamically. I've added that to the build.sh script.

@awholegunch
Copy link
Contributor Author

You asked:
Do we need multiple copies of jquery.js or the cask_404_75pc.png file? Can they not just go under /resources and all the docs that are on the site use them?
Short answer: We need the multiple copies.
Long answer: Each doc set is independent. They are designed to be zipped and then downloaded locally for local use. So each current needs its own copy. They may also change over time, if I ever get around to updating them.

As for the 404 images: they will probably go, but until we have a working deployment on S3 and I can check how the 404 redirects are working, I would like to leave them, and delete them when that is changed.

directory. Added generation of the redirect-page.js from the different
configuration types. Updated the HTML pages and the favicon.ico files.
Updated README file with warning about the Bamboo error.
Copy link
Contributor

@wolf31o2 wolf31o2 left a comment

Choose a reason for hiding this comment

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

After this last set of changes, it 👍 LGTM

for __type in ${__types}; do
__types_array+=", \"${__type}\""
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't an array. It's confusing. Also, you're doing this in the loop, where it doesn't make sense. You already have the complete list, with ${types}, so just use that...

echo "Could not create 'json-versions.js' and 'version' file for ${__type}" ; exit 1)
done

sed -e "s|TYPE_ARRAY|${__types_array:1}|" "${__site}/www/resources/redirect-page.js" > "${__site}/target/www/resources/redirect-page.js" || ( echo "Could not rewrite redirect-page.js to target directory" ; exit 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

sed -e "s|TYPE_ARRAY|$(echo ${types} | sed -e 's#^#"#' -e 's# #" "#g' -e 's#$#"#' -e 's# #, #g')|" ...

Also, that exit won't exit. It'll exit the subshell that you spawned. Instead, remove the || and use:

__ret=$?
[[ ${__ret} -ne 0 ]] && echo "..."
exit ${__ret}

Copy link
Contributor

Choose a reason for hiding this comment

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

$ cat page.js
var something=( TYPE_ARRAY );
sed -e "s|TYPE_ARRAY|$(echo ${types} | sed -e 's#^#"#' -e 's# #" "#g' -e 's#$#"#' -e 's# #, #g')|" page.js
var something=( "cdap", "coopr", "tigon" );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Note that the above needs to be $(echo ${__types}…

j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
'//www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
})(window,document,'script','dataLayer','GTM-KWLFGH');</script>
<!-- Google Tag Manager end -->
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. For some reason I was thinking that it was included in our docs, not in-line, but I think I am just mistaken.

@@ -30,6 +30,8 @@ as they are copies used to test the menus when the configuration file has been g
The build script (see below) copies the contents of ``www`` to the ``target`` directory when
it is run.

**Note:** Due to https://jira.atlassian.com/browse/BAM-18015, we do not use any sub-directories
in the ``resources`` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? This seems like it would belong in a design doc, or as a comment in a file, not in the main README. I'd remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This README is the design doc for this; we don't have one elsewhere. I wanted to make this prominent, so people won't forget.

@wolf31o2
Copy link
Contributor

👍 LGTM

@awholegunch awholegunch merged commit 8e86451 into develop Nov 18, 2016
@awholegunch awholegunch deleted the feature/docs-site-data branch November 18, 2016 22:37
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.

2 participants