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

New portal reindex page. #56

Merged
merged 124 commits into from
Nov 26, 2023
Merged

New portal reindex page. #56

merged 124 commits into from
Nov 26, 2023

Conversation

dmichaels-harvard
Copy link
Contributor

No description provided.

Copy link
Member

@willronchetti willronchetti left a comment

Choose a reason for hiding this comment

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

Comments on the UI are fairly limited but seems functional to me based on integrated testing. Some comments in the back-end may merit addressing or could be converted to tickets for later. Looks great!

Comment on lines +16 to +27
def parse_int(value: Optional[str], fallback: int = 0) -> int:
try:
return int(value) if value else fallback
except ValueError:
return fallback


def parse_bool(value: Optional[str], fallback: int = 0) -> int:
try:
return value.lower() == "true" if value else fallback
except ValueError:
return fallback
Copy link
Member

Choose a reason for hiding this comment

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

These seem like good candidates for utils in the future

Comment on lines 121 to 127
log_events = logs.get_log_events(logGroupName=log_group, logStreamName=log_stream, startFromHead=True)["events"]
for log_event in log_events:
msg = log_event.get("message")
# The entrypoint_deployment.bash script at least partially
# creates this log output, which includes a line like this:
# green: digest: sha256:c1204f9ff576105d9a56828e2c0645cc6dbcf91abca767ef6fe033a60c483f10 size: 7632
if msg and "digest:" in msg and "size:" in msg and (not image_tag or f"{image_tag}:" in msg):
Copy link
Member

Choose a reason for hiding this comment

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

This could be very long... You might want to start from tail as this build digest will be at the very end right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea - I should have thought of that, assumed not supported, but it does seem to be (startFromHead=False)

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 should/will add a comment to this effect but this fetching of the digest is not critical to the functioning of the page - it just gives a nice verification that the most recent build does in fact match the current image.

Comment on lines +115 to +116
# For some reason this (rarely-ish) intermittently fails with no error;
# the results just do not contain the digest; don't know why so try a few times.
Copy link
Member

Choose a reason for hiding this comment

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

Failed build potentially?

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, it happens on successful builds too, it is intermittent, who knows - search from the end like you suggest might help, which I did make that change.

Comment on lines +296 to +300
def get_aws_ecr_build_info(image_repo_or_arn: str, image_tag: Optional[str] = None, previous_builds: int = 2) -> Dict:
"""
Returns a dictionary with info about the three most recent CodeBuild builds
for the given image repo and tag, or None if none found.
"""
Copy link
Member

Choose a reason for hiding this comment

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

This function among others in this module probably belong in utils, something to think about in the future if you have the time

if len(vpcs) == 1:
vpc = vpcs[0]
else:
vpcs = [item for item in vpcs if "main" in (item.get("name") or "").lower()]
Copy link
Member

Choose a reason for hiding this comment

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

You might want to comment that this "main" identifier is very important and specific to our infrastructure code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const region = "us-east-1";

function awsClusterLink(id) {
return `https://${region}.console.aws.amazon.com/ecs/v2/clusters/${id}/services?region=${region}`;
Copy link
Member

Choose a reason for hiding this comment

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

You might want to make the base URL here a constant so if it changes (which it has since we started using ECS) you can update more easily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +247 to +255
{ (props.cluster?.env?.is_production || props.cluster?.env?.is_staging || props.cluster?.env?.color) &&
<small style={{float: "right", color: props.cluster?.env?.color == "blue" ? "blue" : (props.cluster?.env?.color == "green" ? "green" : "")}} onClick={toggleShowDetail}>
{props.cluster?.env?.is_production && <b>PRODUCTION</b>}
{props.cluster?.env?.is_staging && <b>STAGING</b>}
{props.cluster?.env?.color && <>
&nbsp;({props.cluster?.env?.color?.toUpperCase()})
</>}
</small>
}
Copy link
Member

Choose a reason for hiding this comment

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

Are we handling the standalone case cleanly as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

</td></tr></tbody></table>
{ isShowUpdatingWarning() && (!isShowUpdatingButton() || confirmed) && !runResult && <small style={{color: "red"}}>
<SeparatorH color="red" />
<b>Warning</b>: A cluseter update appears to be already <u><b>running</b></u>. Run this <u><b>only</b></u> if you know what you are doing!
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "cluster"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

const region = "us-east-1";

function awsTaskRunLink(id) {
return `https://${region}.console.aws.amazon.com/ecs/v2/task-definitions/${id}/run-task`;
Copy link
Member

Choose a reason for hiding this comment

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

Same comment re constants as in the last file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +69 to +75
const taskNames = [
{ name: "deploy", label: "Reindex" },
{ name: "deploy_initial", label: "Initial Deploy" },
{ name: "indexer", label: "Indexer" },
{ name: "ingester", label: "Ingester" },
{ name: "portal", label: "Instance" }
];
Copy link
Member

Choose a reason for hiding this comment

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

These map directly to values in Python, should probably be noted (and made constants in the back-end as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this driven by table of regex and put in only module (aws_ecs_types.py) for easy finding.

@dmichaels-harvard dmichaels-harvard merged commit aa3b49d into master Nov 26, 2023
1 of 2 checks passed
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