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

Update schemas with build_avatar #818 #1759

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sagar-pathak
Copy link

@sagar-pathak sagar-pathak commented Feb 17, 2025

Description of proposed changes

Schema information added for build_avatar in schema-annotations.json and schema-export-v2.json file.

Related issue(s)

#818

Checklist

  • Automated checks pass
  • Check if you need to add a changelog message
  • Check if you need to add tests
  • Check if you need to update docs

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

The schema changes look ok, pending the review comment.

Note that this doesn't allow the build_avatar, if set on the auspice-config JSON, to be exported via augur export v2. To achieve that we'd need to extend export_v2.py to pass this through, similar to how it parses build_url (although we wouldn't need to expose this via a command-line arg):

augur/augur/export_v2.py

Lines 1075 to 1080 in 23b9fff

def set_build_url(data_json, config, cmd_line_build_url):
# build_url is not necessary. Cmd line args override any config settings
if cmd_line_build_url:
data_json['meta']['build_url'] = cmd_line_build_url
elif config.get("build_url"):
data_json['meta']['build_url'] = config.get("build_url")

Comment on lines 32 to 36
"build_avatar": {
"description": "The custom avatar image URL allows users to include a group-specific logo, even if it is hosted outside of GitHub.",
"$comment": "Auspice displays this at the top of the page as part of a byline",
"type": "string"
},
Copy link
Member

Choose a reason for hiding this comment

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

This should follow the form of build_url and reference the definition in the auspice-config to avoid duplication.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @jameshadfield, I will work on this.

Copy link
Member

Choose a reason for hiding this comment

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

Also, one more tiny change - can you remove the word "group-specific" - we already have a concept of "Nextstrain groups" which are unrelated to this and I don't want to potentially confuse some people

Copy link
Author

@sagar-pathak sagar-pathak Feb 18, 2025

Choose a reason for hiding this comment

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

I am curious if we really need the else block, since this field is optional. Let me know. @jameshadfield

def set_build_avatar(data_json, config):
    # build_avatar is not necessary. No cmd line args handling
    if config.get("build_avatar"):
        data_json['meta']['build_avatar'] = config.get("build_avatar")

Copy link
Member

Choose a reason for hiding this comment

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

That code snippet looks good. (We don't need the else block because we're not exposing this as a command-line option to augur export)

Copy link
Author

Choose a reason for hiding this comment

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

@jameshadfield I am not sure why the build is failing. Do you have any idea on how to fix this one?

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.

2 participants