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

Accessing and modifying description and labels of Enso Cloud assets #11255

Merged
merged 36 commits into from
Oct 10, 2024

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Oct 4, 2024

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@radeusgd radeusgd self-assigned this Oct 4, 2024
@radeusgd radeusgd force-pushed the wip/radeusgd/11227-cloud-asset-metadata branch from a0210bf to edfde57 Compare October 5, 2024 16:31
@radeusgd radeusgd marked this pull request as ready for review October 5, 2024 16:44
Context.Output.if_enabled disabled_message="Cannot remove a label when writing is disabled. Press the Write button ▶ to perform the operation." panic=False <|
labels = self.labels
if labels.contains label . not then False else
new_labels = labels.filter l-> l != label
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have this as Vector.remove_element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but adding it I'd have to add unit tests for it and I don't want to postpone this PR.

Let's add it in a separate PR when we see it used more, or see a need for it for end-users.

@radeusgd radeusgd force-pushed the wip/radeusgd/11227-cloud-asset-metadata branch from 4fd50b7 to 8b677f9 Compare October 7, 2024 16:22
@radeusgd radeusgd force-pushed the wip/radeusgd/11227-cloud-asset-metadata branch from 8b677f9 to 28a2e5a Compare October 7, 2024 16:23
# Conflicts:
#	distribution/lib/Standard/Base/0.0.0-dev/src/Enso_Cloud/Internal/Existing_Enso_Asset.enso
mergify bot pushed a commit that referenced this pull request Oct 9, 2024
- While #11255 fixes the root cause of #11278, this PR fixes what triggered it - since #11198 our runners were keeping `~/.enso/credentials` file between runs, meaning that some tests were unexpectedly running in 'authenticated' mode. This PR cleans up this file to avoid that.
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Generally looks good - a few suggestions.

Comment on lines 199 to 204
Context.Output.if_enabled disabled_message="Cannot add a label when writing is disabled. Press the Write button ▶ to perform the operation." panic=False <|
labels = self.labels
if labels.contains label then self else
ensure_tags_exist [label] . if_not_error <|
new_labels = labels+[label]
update_asset_labels self new_labels . if_not_error self
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Context.Output.if_enabled disabled_message="Cannot add a label when writing is disabled. Press the Write button ▶ to perform the operation." panic=False <|
labels = self.labels
if labels.contains label then self else
ensure_tags_exist [label] . if_not_error <|
new_labels = labels+[label]
update_asset_labels self new_labels . if_not_error self
labels = self.labels
if labels.contains label then self else
ensure_tags_exist [label] . if_not_error <|
new_labels = labels+[label]
Context.Output.if_enabled disabled_message="Cannot add a label when writing is disabled. Press the Write button ▶ to perform the operation." panic=False <|
update_asset_labels self new_labels . if_not_error self

Could make it only check the context as last step then allow user to fix issue

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we shouldn't change the order. Perhaps I should rename the ensure_tags_exist method to make it clearer.

This method creates tags if they don't exist. So it is a method with side-effects, that calls into the Cloud. Hence, it itself should be behind the Output context.

I will rename the method to create_tags_if_not_exist to make it clearer that this is more than a pre-check.

Copy link
Member Author

@radeusgd radeusgd Oct 10, 2024

Choose a reason for hiding this comment

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

But I can indeed move the contains check before context check 👍

Sorry at first I did not notice this part

Adding labels is not atomic. If two processes are modifying labels of
the same asset at the same time, some changes may be lost.
add_label : Text -> Enso_File
add_label self (label : Text) -> Enso_File =
Copy link
Member

Choose a reason for hiding this comment

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

worth routing this through set_labels?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of it, but then the message if Context was disabled would be 'Cannot set labels...', but I wanted a more appropriate/precise 'Cannot add a label'.

Comment on lines 257 to 258
Context.Output.if_enabled disabled_message="Cannot create a label when writing is disabled. Press the Write button ▶ to perform the operation." panic=False <|
create_tag name color . if_not_error Nothing
Copy link
Member

Choose a reason for hiding this comment

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

worth testing existance?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Although ideally that should be checked on the cloud side and only a special error returned.

Comment on lines 119 to 121
## PRIVATE
new_with_metadata id:Text title:Text metadata:Asset_Metadata -> Existing_Enso_Asset =
Existing_Enso_Asset.Value title id (asset_type_from_id id) metadata
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## PRIVATE
new_with_metadata id:Text title:Text metadata:Asset_Metadata -> Existing_Enso_Asset =
Existing_Enso_Asset.Value title id (asset_type_from_id id) metadata
## PRIVATE
new id:Text title:Text metadata:(Nothing|Asset_Metadata)=Nothing -> Existing_Enso_Asset =
Existing_Enso_Asset.Value title id (asset_type_from_id id) metadata

Could we not have a single new here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make it very clear when the asset is constructed with or without metadata. IMO it is a bit clearer when this is verbose. But I can merge them - initially there were more differences in the logic, now indeed it's very similar

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 10, 2024
@mergify mergify bot merged commit 3458fe4 into develop Oct 10, 2024
36 checks passed
@mergify mergify bot deleted the wip/radeusgd/11227-cloud-asset-metadata branch October 10, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Often random failures of Audit_Spec on the CI Access metadata of Enso Cloud assets programmatically
3 participants