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

Workload entry expiry features #96

Open
wants to merge 1 commit into
base: main-old
Choose a base branch
from

Conversation

maia-iyer
Copy link
Collaborator

editing entry-create form for multiple entry formats and warnings on already expired entries or data that causes no expiry date

@maia-iyer maia-iyer self-assigned this Aug 4, 2021
@maia-iyer maia-iyer linked an issue Aug 5, 2021 that may be closed by this pull request
3 tasks
@maia-iyer maia-iyer force-pushed the workload-entry-expiry-features branch 2 times, most recently from 23c839f to bbcc590 Compare August 9, 2021 19:27
@maia-iyer maia-iyer force-pushed the workload-entry-expiry-features branch from 265d67d to 3100b66 Compare August 10, 2021 18:24
Copy link
Collaborator

@mamy-CS mamy-CS left a comment

Choose a reason for hiding this comment

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

Other than these, lgtm!

tornjak-frontend/src/components/style.css Outdated Show resolved Hide resolved
tornjak-frontend/src/components/style.css Outdated Show resolved Hide resolved
tornjak-frontend/src/components/entry-create.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/entry-create.js Outdated Show resolved Hide resolved
});
}

isValidJSTime(seconds) {
Copy link
Owner

Choose a reason for hiding this comment

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

this is not just isValidJSTime since it asserts >0, so this is more specific to the usecase, so isValidExpiryTime may be more appropriate, also while checking this, it may be good to also see if there's an upper limit on SPIRE type as well, i think it probably is 64 bit so just verify that that upper bound is checked too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so from whatever I could find/tell, you're right, that SPIRE deals with 64bit for time. Then, the upper limit on SPIRE is a bit larger (about .5e15) than the JS upper bound on dates - is this still then a necessary check?

tornjak-frontend/src/components/entry-create.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/entry-create.js Outdated Show resolved Hide resolved
@@ -115,6 +115,9 @@ class SpiffeHelper extends Component {
// n:1, this would reduce the total cost. This may be useful when
// performance is impacted.
getAgentsEntries (agents, entries) {
if (typeof entries === 'undefined') {
return {};
Copy link
Owner

Choose a reason for hiding this comment

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

im kind of curious what triggered this, this is actually a fault in the caller making this call, so we should really resolve that instead, can we also console.log a message here, and be like "getAgentEntries: this should not happen, entries undefined"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should I remove the check then? I found the error (in dashboard when there are no workload entries) and have added a todo statement in the relevant line of code in dashbard/agents-dashboard-table.js

Copy link
Collaborator

@mamy-CS mamy-CS left a comment

Choose a reason for hiding this comment

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

Good to be merged! Small tweaks can be done after the repo move.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workload Entry Expiry Time UI Handling
3 participants