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

Enable message of the day #140

Merged
merged 2 commits into from
Aug 21, 2020
Merged

Enable message of the day #140

merged 2 commits into from
Aug 21, 2020

Conversation

ManInFez
Copy link
Contributor

@ManInFez ManInFez commented Aug 7, 2020

@ManInFez ManInFez self-assigned this Aug 7, 2020
@pinkwah
Copy link
Collaborator

pinkwah commented Aug 19, 2020

This is really good. I think it's better if the script goes over every release in /prog/res/komodo and matches them.

If this script goes over every release, then we'll be able to do things like apply messages to stable, which will just be put in whatever stable points to.

A rule like the following should apply to 2020.08.01-py27 if that is pointed to by stable-py27.

'stable*':
  messages:
    - thank-you-for-using-stable.txt

For a rule like

'*-py27':
  messages:
    - py27-is-deprecated.txt

could match both stable-py27 and 2020.01.00-py27 that stable-py27 points to. In that case only 1 such message should be added.

@pinkwah
Copy link
Collaborator

pinkwah commented Aug 19, 2020

Also, do we want to have messages inline in the YAML file? It'll cut down on the number of files in git for trivial messages.

"--komodo-prefix",
"-k",
required=True,
help="Path to folder holding komodo-releases.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ambiguous: Path to https://github.com/equinor/komodo-releases or /prog/res/komodo? Perhaps "Path to komodo releases root (eg. /prog/res/komodo)". Also make it the default.

Copy link
Collaborator

@pinkwah pinkwah left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 16 to 20
scripts = (
scripts + motd_db[key].get("scripts")
if motd_db[key].get("scripts") is not None
else scripts
)
Copy link
Contributor

@sondreso sondreso Aug 20, 2020

Choose a reason for hiding this comment

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

scripts.extend(motd_db[key].get("scripts", []))?



def copy_files(file_list, dst_path, src_path):
if os.path.isdir(dst_path):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should not happen, as they are cleared in the beginning



def create_inline_messages(messages, clear_old_messages, dst_path):
if clear_old_messages:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should not happen, as they are cleared in the beginning

Copy link
Contributor

@sondreso sondreso left a comment

Choose a reason for hiding this comment

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

Except for the comments this LGTM as well 🚀

Copy link
Contributor

@sondreso sondreso left a comment

Choose a reason for hiding this comment

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

Still LGTM 🚀

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.

Make sure users are warned if running with /project/res in path
3 participants