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

ReservedPackage + admin action. #8105

Merged
merged 4 commits into from
Oct 7, 2024
Merged

ReservedPackage + admin action. #8105

merged 4 commits into from
Oct 7, 2024

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Oct 4, 2024

@isoos isoos requested review from jonasfj and sigurdm October 4, 2024 15:13
''',
options: {
'package': 'The package name to be reserved.',
'emails': 'The list of email addresses, separated by comma.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an option to delete the entity? Maybe it is not worth it...

Copy link
Member

Choose a reason for hiding this comment

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

We could just add:

  • package-reserve-list
  • package-reserve-delete

Life is easier, if we split them into multiple actions. We can still put them all in the same file if want that.

We could even consider naming them:
package-reservation-create
package-reservation-list
package-reservation-delete

naming is a nit, but it's probably more intuitive to have separate options. And it's probably easier to read the code 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.

Like a --delete flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - probably --delete=true as we didn't implement real flags for actions,

Copy link
Member

Choose a reason for hiding this comment

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

a separate command is MUCH easier to do.

Copy link
Member

Choose a reason for hiding this comment

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

it's really just something like:

final package = options['package'];
InvalidInputException.check(
  package != null && package.isNotEmpty,
  '`package` must be given',
);

await withRetryTransaction(dbService, (tx) async {
  final existing = await tx.lookupOrNull<ReservedPackage>(
      dbService.emptyKey.append(ReservedPackage, id: package));
      
  if (exiting == null) {
    return {'message': 'ReservedPackage for "$package" does not exist!'};
  }
  
  tx.delete(existing.key);
});

return {'message': 'ReservedPackage for "$package" deleted.'};

A --delete=true flag requires a lot more explanation in the documentation, it's not very intuitive.

app/lib/admin/actions/package_reserve.dart Outdated Show resolved Hide resolved
''',
options: {
'package': 'The package name to be reserved.',
'emails': 'The list of email addresses, separated by comma.'
Copy link
Member

Choose a reason for hiding this comment

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

We could just add:

  • package-reserve-list
  • package-reserve-delete

Life is easier, if we split them into multiple actions. We can still put them all in the same file if want that.

We could even consider naming them:
package-reservation-create
package-reservation-list
package-reservation-delete

naming is a nit, but it's probably more intuitive to have separate options. And it's probably easier to read the code too.

app/lib/package/backend.dart Outdated Show resolved Hide resolved
@sigurdm
Copy link
Contributor

sigurdm commented Oct 7, 2024

Yeah - multiple commands is probably better.

@isoos
Copy link
Collaborator Author

isoos commented Oct 7, 2024

Updated, PTAL.

@isoos
Copy link
Collaborator Author

isoos commented Oct 7, 2024

I will add list/delete in a follow-up PR.

@isoos isoos merged commit 91d82f8 into dart-lang:master Oct 7, 2024
32 checks passed
@isoos isoos deleted the reserve-pkg branch October 7, 2024 09:53
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.

Admin action for reserving package names
3 participants