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

Rewrite of cluster lesson #73

Conversation

psteinb
Copy link
Contributor

@psteinb psteinb commented Dec 17, 2018

  • pushing the story to full bloom (just before using the scheduler)
  • correcting mistakes on prefixes on Byte counts (gibi versus giga)
  • refresh on ssh/scp

@psteinb psteinb changed the title Rewrite of scheduler lesson Rewrite of cluster lesson Dec 17, 2018
_episodes/12-cluster.md Outdated Show resolved Hide resolved
_episodes/12-cluster.md Outdated Show resolved Hide resolved
_episodes/12-cluster.md Outdated Show resolved Hide resolved
@Carreau
Copy link
Contributor

Carreau commented Dec 17, 2018

That looks good to me. There are probably some possible refinement, but it feel like a better gradual approach.

@psteinb
Copy link
Contributor Author

psteinb commented Dec 19, 2018

@Carreau please have a look at that single review comment I left unresolved for now. Would be cool, to merge soon.

_episodes/12-cluster.md Outdated Show resolved Hide resolved
@Sabryr
Copy link
Contributor

Sabryr commented Jan 30, 2019

What is the status of this PR ? , if my latest PR (Fix styling fix#35) is integrated then this one will face conflicts.

@psteinb
Copy link
Contributor Author

psteinb commented Jan 30, 2019 via email

@tkphd
Copy link
Collaborator

tkphd commented Jan 30, 2019

Thanks, @psteinb. Merge conflicts are natural, @Sabryr, and enforcing the 100-character alignment rule should not be a great challenge regardless of the merge order.

I use meld to resolve merge conflicts in my local copy of the repository. It comes in handy.

@psteinb
Copy link
Contributor Author

psteinb commented Feb 1, 2019

As @Sabryr is waiting upstream for this PR, @tkphd @Carreau could you please have a look again and re-review or merge, please.

@aturner-epcc
Copy link
Contributor

I think we merge this one and plan who is doing what with regards to automatic includes going forwards.

Copy link
Collaborator

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

Confirmed: this change produces lola@cray-1. LGTM.

Optional suggestion is to remove workshop_ from the variable names before merging.

_config.yml Outdated Show resolved Hide resolved
@psteinb psteinb merged commit d86fca2 into carpentries-incubator:gh-pages Feb 5, 2019
@psteinb
Copy link
Contributor Author

psteinb commented Feb 5, 2019

Thanks to @tkphd @Carreau for their reviews. I think this pushed the quality of this PR upwards quite extensively. It's wonderful to see the community helping to create some great material for HPC training.

@Sabryr @aturner-epcc Ready to go downstream.

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.

None yet

5 participants