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

4.0 Release Notes Feedback Part 2 #1049

Open
55 of 67 tasks
moffatsadeghi opened this issue Dec 27, 2024 · 12 comments
Open
55 of 67 tasks

4.0 Release Notes Feedback Part 2 #1049

moffatsadeghi opened this issue Dec 27, 2024 · 12 comments
Assignees

Comments

@moffatsadeghi
Copy link
Contributor

moffatsadeghi commented Dec 27, 2024

  • Image One and Feedback

image

  • - With my suggestions of the Table of Contents, "Breaking Changes" would be the new header instead of "Details of administrative changes."
  • - In my previous issue, 4.0 Release Notes Feedback Part 1 #1045, I suggested the sentence should be "Configurations whitelist & blacklist have been removed," but I don't think it matters much. I do think that we should include the "why" for the change - more inclusive wording.
  • "These configurations that been updated to uses more inclusive language." Should be "These configurations been updated to use more inclusive language." Grammar change.

  • Image Two and Feedback

image

  • Announcements are also required and I would like to call that out here as well. I think it's fine to link more details, but we should call all the enhancements to Announcements this release.
  • I also recommend that announcements move to "New Features and Enhancements" since this seems more of an enhancement to what was previously there rather than a "Breaking Change." I am definitely open to hearing your thoughts on this too.
  • Correcting dismissable to dismissible.
  • See commas and periods on the screenshot to where I'd recommend putting a comma or period.
  • Capitalize circled words.
  • I've crossed some words out that are redundant or unnecessary (I know this is a tiny tweak and if you don't want to touch it, it is fine.)
  • I think it is "root-owned" rather than "root owned."
  • In my previous issue, I recommended changing the title from "Root owned configuration files" to "Root-owned configuration files have changed."
  • Change "This will mean that..." to "This means that..." again, small change, but I think it reads better the second way.

  • Image Three and Feedback

image

  • Capitalize circled words.
  • "Every OS" sounds better than "All OSes", but that might be negligible.
  • "Ruby-based" and "Node-based"
  • "...supported on ppc64le because NodeJS 20 is not available on that architecture" reads better.
  • Will the passenger, nginx, or ondemand-dex bullets receive information? If not, let's get rid of them.
  • Do the underlined words in the last warning need specialized font? Like "bin/ruby" in the first warning?
  • Anything going under "SELinux changes?"
  • Do the "Dependency Updates" call the readers to action? Do they need to update any dependencies or we have handled that on their behalf?

  • Image Four and Feedback

image

  • Add "Table of Contents:" above the bullet points
  • Add "Released on January..." below "v4.0 Release Notes"
  • Replace "Thanks!" with "Acknowledgements"
  • Replace "Upgrade Directions with Upgrade Instructions"
  • Move "Upgrade Instructions" to the end. Reason: the flow is better to cover everything included in the new release and then instruct folks to upgrade. Including it in the middle before features is strange to me.
  • Replace "Details of New Features" to "New Features and Enhancements"
  • Is there anything security related that we should call out and add a section on?

  • Image Five and Feedback

image

  • Add something to the second orange box to be more specific. "We have tested the upgrade from 3.1.10 to 4.0.0 at OSC's OnDemand instance." or something like that. The current sentence is too vague.
  • Overall, my thoughts on this section is that I see what you are trying to do, but I'd like for more consistency. For example, the two bracketed steps in 2 and 4 are inconsistent. In two, you have a bolded sentence below that indicates certain OSes need to complete that step. In four, you have "(Optional)" to indicate that certain sites need to complete that step if using Dex. My recommendation is consistency - chose either bolded below to call out a special use case OR use "(Optional if...).
  • Replace "Dex based" with "Dex-based"
  • Replace "authentiction" with "authentication"

  • Image Six and Feedback

image

  • I love that we are including upgrade instructions in the release notes!! I think this is a win for the community!!
  • Add periods at the end of these sentences as illustrated on the screenshot.
  • Step 5 is confusing to me. What is the top section and what is the bottom? Is one the update config and the second is the restart? Why not make them two separate steps? Especially considering that one is universal and the other is optional depending on the OS. Make it consistent to the rest of the steps and the feedback above.
  • Add "Linux" between Amazon and 2023 for consistency.
  • All the stared (*) sections tie into my feedback for image 5 on consistent wording / style for these instructions.
  • Add a space between steps 7 and 8 because its a bit crammed.
  • I don't like linking in step 7 because we are being pretty thorough in the directions. I think we should include how to do step 7 in these directions. If you want to keep the link there, maybe it could be for more information. Linking off the page during this step by step could confuse the reader walking through this.
  • How can someone check or know step 9? How can they know if other applications are using old dependencies?
  • In step 10, are "mod_auth_openidc" and "cjose" supposed to be in red text seen in steps 6 and 7 for example?

  • Image 7 and Feedback

image

  • Add commas as shown in the screenshot.
  • After the first sentence, it would be helpful to briefly share what we did before. Something like, "Previously, form items were..."
  • noVNC Quality and Compression Defaults: Either in those links or in the release notes, I'd recommend adding some context to why this is needed or helpful. I could see some people knowing why this is important or helpful and others not knowing what this is.
  • Change "browswer" to "browser"
  • Change "...change this had do use the..." to "...change this had to use the..." Do -> to.
  • The underlined sentence under batch connect, I don't understand what you are saying - is poll delay being deprecated, the documented configuration is being deprecated, or something else? Either the wording needs changed or some context needs added.
  • For consistency, hyperlink "system status application" instead of providing the whole GitHub link.
  • Add a "y" in the hyperlink text "the documentation on status_poll_dela" so its now "the documentation on status_poll_delay"

  • Image 8 and Feedback

image

  • Capitalize circled letters. The "now"s need to be capitalized because they are not articles like to, a, the, etc.
  • A sentence of context to what data-hide directives would be helpful. And/or why this change is positive.
  • Spell out "UIDs" in the title
  • A sentence of context or further description would be helpful for the user mapping section.
  • "Intereative" to "interactive"
  • "becuase" to "because"
  • Should be "RHEL-based systems"
  • Should be "set up" instead of "set up"
  • A sentence of context or further description would be helpful for the XDMOD section. I believe this is an update from Aday so I have asked him to provide a blurb to further describe this.
  • A sentence of context or further description would be helpful for the saved settings section. Did Aday do this? If so, I can ask him about this too.
  • For the very last sentence, this flows better, "This change ensures that centers that delete users (remove them from LDAP) will also clean up related processes on the OnDemand machine."
@johrstrom
Copy link
Contributor

I updated the comment slightly to have a task list. I've got capitalization in headers in flight in #1050.

@johrstrom
Copy link
Contributor

johrstrom commented Dec 30, 2024

I have #1051 open with most of these updates that I've checked off. You'll be able to see the updates at https://osc.github.io/ood-documentation-test/more-40-updates/release-notes/v4.0-release-notes.html once it publishes, which will be a few minutes from now.

Here are my comments to some of the tasks:

Will the passenger, nginx, or ondemand-dex bullets receive information? If not, let's get rid of them.

Not sure what you mean. These bullets are the current versions of these dependencies in 4.0.
Emily's response: then let's move them to the beginning of that section. Those 3 bullets are mixed into those orange warning boxes and it looks unorganized as a result.

Do the underlined words in the last warning need specialized font? Like "bin/ruby" in the first warning?

I don't see any underlines here, but I'd be happy to apply bold or italics to something.
Emily's response:
image
I meant these 2 words. Do they need to appear on the page in red text like "bin/ruby" right above it?

Anything going under "SELinux changes?"

I need to find out and fill that section out.

Do the "Dependency Updates" call the readers to action? Do they need to update any dependencies or we have handled that on their behalf?

We update things on their behalf, but all the warning sections are stuff they may need to do.
Emily's response: If they need to take action, can we link them directions on how to do that or provide directions on the stuff they need to change?

Is there anything security related that we should call out and add a section on?

I'll have to look into it.

Overall, my thoughts on this section is that I see what you are trying to do, but I'd like for more consistency.

I see what you've mean. I've done ([Reasons] only), we can continue to refine it.

How can someone check or know step 9? How can they know if other applications are using old dependencies?

When they issue dnf remove dnf itself will error if they're still actively using it.

johrstrom added a commit that referenced this issue Dec 30, 2024
More 4.0 updates after the feedback given in #1049.
@johrstrom
Copy link
Contributor

I merged the updates just now. You should be able to see all these items updated in a few minutes.

@moffatsadeghi
Copy link
Contributor Author

Thank you for updating! It is really coming along! I will review your responses and get back to you!

@moffatsadeghi
Copy link
Contributor Author

moffatsadeghi commented Dec 30, 2024

Ignore for now

  • "Meaning users can press OK on the announment and it will no longer appear on the pages." Should be "Meaning users can press OK on the announcement and it will no longer appear on the pages."

@moffatsadeghi
Copy link
Contributor Author

@johrstrom I've responded to 3 things in your comment just below the pull request.

@johrstrom
Copy link
Contributor

I had a comment here that I did not save apparently...

In any case, while inlining is fine, what folks typically do to start a thread like thing in github is to do something like this.

> original
>> response
>>> OP response

That then appears like this

Do the "Dependency Updates" call the readers to action? Do they need to update any dependencies or we have handled that on their behalf?

We update things on their behalf, but all the warning sections are stuff they may need to do.

Emily's response: If they need to take action, can we link them directions on how to do that or provide directions on the stuff they need to change?

I filed #1055 in response to this.

@johrstrom
Copy link
Contributor

Will the passenger, nginx, or ondemand-dex bullets receive information? If not, let's get rid of them.

I kinda think we should keep these. Some centers are very interested in these dependencies. They won't get bullets because there's really nothing more to say about them.

IDK I think it's a good idea to continue to communicate these dependencies as they sometimes get large/popular vulnerabilities.

@moffatsadeghi
Copy link
Contributor Author

Will the passenger, nginx, or ondemand-dex bullets receive information? If not, let's get rid of them.

I kinda think we should keep these. Some centers are very interested in these dependencies. They won't get bullets because there's really nothing more to say about them.

IDK I think it's a good idea to continue to communicate these dependencies as they sometimes get large/popular vulnerabilities.

That is fine if we keep them, but I revised my thoughts with this sentence: "then let's move them to the beginning of that section. Those 3 bullets are mixed into those orange warning boxes and it looks unorganized as a result."

@johrstrom
Copy link
Contributor

That is fine if we keep them, but I revised my thoughts with this sentence: "then let's move them to the beginning of that section. Those 3 bullets are mixed into those orange warning boxes and it looks unorganized as a result."

👍

@moffatsadeghi
Copy link
Contributor Author

Updated comment with final feedback notes. After we make PRs for all the changes, I plan on reviewing it again to catch anything else.

@moffatsadeghi
Copy link
Contributor Author

moffatsadeghi commented Jan 3, 2025

Add new features / enhancements:

  • Announcements dismissible and required
  • File editor enhancements

Acknowledgements

  • Grab GitHub usernames
  • Call out Harvard
  • Call out CSC Finland

Further task items from conversation with Emily and Jeff

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

No branches or pull requests

2 participants