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

feat: extend manifest integration test #443

Merged
merged 13 commits into from
Jan 30, 2025
Merged

Conversation

acha-bill
Copy link
Contributor

Uses feeds to test updates to a website

  1. It creates a root feed manifest
  2. To make an update to the feed, it uploads a tar website with some index.html and other files. Then uses this reference in the feed update.
  3. It then tries to download the website via the root feed reference. It should resolve to the index.html
  4. To update the website, we push another update to the feed with the new tar and then try to download it again via the root feed reference.
  5. Lastly, it tries to download the other files in the website directly. e.g b25c89a401d9f26811680476619a1eb4a4e189e614bc6161cbfd8b343214917b/assets/styles/styles.css

@istae istae requested a review from martinconic January 16, 2025 11:28
@gacevicljubisa
Copy link
Contributor

This part of code should be defered when buffer initialized

if err := tw.Close(); err != nil {
return nil, err
}

It will ensure that io.Writer is properly closed even if error occurs during writing.

pkg/check/manifest/manifest.go Outdated Show resolved Hide resolved
pkg/check/manifest/manifest.go Outdated Show resolved Hide resolved
pkg/check/manifest/manifest.go Outdated Show resolved Hide resolved
@gacevicljubisa
Copy link
Contributor

gacevicljubisa commented Jan 23, 2025

One improvement regarding Options:

func NewDefaultOptions() Options {
return Options{
FilesInCollection: 10,
GasPrice: "",
MaxPathnameLength: 64,
PostageAmount: 1,
PostageDepth: 16,
PostageLabel: "test-label",
Seed: 0,
}
}

Maybe to put PostageLabel default value to manifest-check, or something like that to distingush it from other batch labels.
Maybe this would be nice to do for every check that uses PostageLabel option, to have the check name in it? What do you think @istae ?

Also, MaxPathnameLength shouldn't be less then 2, or it will panic.

@istae
Copy link
Member

istae commented Jan 23, 2025

One improvement regarding Options:

func NewDefaultOptions() Options {
return Options{
FilesInCollection: 10,
GasPrice: "",
MaxPathnameLength: 64,
PostageAmount: 1,
PostageDepth: 16,
PostageLabel: "test-label",
Seed: 0,
}
}

Maybe to put PostageLabel default value to manifest-check, or something like that to distingush it from other batch labels.
Maybe this would be nice to do for every check that uses PostageLabel option, to have the check name in it? What do you think @istae ?
Also, MaxPathnameLength shouldn't be less then 2, or it will panic.

The label can be used strategically in general, because the GetCreateMutableBatch function matches the label, and it can reuse the same batch created before to save on cost and time (by not waiting for a new batch to be created which can take few minutes).

@gacevicljubisa
Copy link
Contributor

gacevicljubisa commented Jan 23, 2025

The label can be used strategically in general, because the GetCreateMutableBatch function matches the label, and it can reuse the same batch created before to save on cost and time (by not waiting for a new batch to be created which can take few minutes).

@istae Would this mean that we can resuse the same batch created with specific check? Or is it ok to use test-label for all of the checks?
What is your suggestion, to leave the same label for all of the checks?

@istae
Copy link
Member

istae commented Jan 23, 2025

The label can be used strategically in general, because the GetCreateMutableBatch function matches the label, and it can reuse the same batch created before to save on cost and time (by not waiting for a new batch to be created which can take few minutes).

@istae Would this mean that we can resuse the same batch created with specific check? Or is it ok to use test-label for all of the checks? What is your suggestion, to leave the same label for all of the checks?

If the tests run one by one then it's more optimal to simply reuse the same batch.

@istae istae requested a review from janos January 23, 2025 23:14
pkg/check/manifest/manifest.go Outdated Show resolved Hide resolved
pkg/check/manifest/manifest.go Outdated Show resolved Hide resolved
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

I agree with existing comments and would like to add that the check will be more responsive in certain situations if time.Sleep is replaced with select with cases on ctx.Done and time.After.

@acha-bill acha-bill merged commit a02803a into master Jan 30, 2025
3 checks passed
@acha-bill acha-bill deleted the feat/extended-manifest branch January 30, 2025 07:47
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.

5 participants